-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(x/ecocredit): add simpler version of send
command
#1213
feat(x/ecocredit): add simpler version of send
command
#1213
Conversation
hi @ryanchristo and @technicallyty, this is still WIP so no need for an in-depth review, but i'm somewhat new to Go and to this project so wanted to check in and make sure this is on the right track. i still need to implement the also, i found my self getting confused with |
Codecov Report
@@ Coverage Diff @@
## master #1213 +/- ##
==========================================
+ Coverage 80.02% 80.09% +0.06%
==========================================
Files 232 232
Lines 21533 21626 +93
==========================================
+ Hits 17232 17321 +89
- Misses 2988 2990 +2
- Partials 1313 1315 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
hey thanks for the contribution! the intent of the issue was to only add a new CLI command (so no additional protobuf messages/rpc methods), and to use the existing |
ahhh, i see. yeah that totally makes sense. i'll take another crack at it, thanks! |
9a25fcb
to
a50a9e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for another contribution!
This is looking good. And it looks like you got some experience with proto even if that's not what was needed for this pull request. We just made some changes to the transaction commands so it looks like there will be a few conflicts to resolve. It would be great to add a test as well, which should be pretty straightforward using an existing transaction command test as an example.
my pleasure! thanks for the detailed feedback, i'll get another rev of this up for review in the next few days |
…ng a single credit type fixes regen-network#1023
- add integration tests for new command
a50a9e7
to
2510b4c
Compare
…le-send-credit-command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work! just a few nits and then this should be good to go! 🎉
thanks! all comments addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. Just a couple small changes and then I think it's good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks again for the contribution @sfishel18 ! 🎉
send
command for exchangi…send
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - thanks!
Description
Closes: #1023
Renames the existing
send
command tosend-bulk
, and adds implementation of a simplersend
command that can only handle a single transaction of a single credit type but does not require YAML arguments.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change