-
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
refactor(x/ecocredit): update location to jurisdiction #1020
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1020 +/- ##
=======================================
Coverage 48.93% 48.93%
=======================================
Files 225 225
Lines 23442 23442
=======================================
Hits 11472 11472
+ Misses 10724 10722 -2
- Partials 1246 1248 +2
Flags with carried forward coverage won't be shown. Click here to find out more. |
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 like Location
is still showing up in a few places.
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. I'm seeing "location" in a few comments and test cases that would be nice to update. Pre-approving.
x/ecocredit/core/msg_create_batch_test.go
x/ecocredit/core/msg_mint_batch_credits.go
x/ecocredit/core/msg_retire_test.go
x/ecocredit/server/core/retire.go
Also looks like another case of random string used in message validation tests causing a race condition: https://github.com/regen-network/regen-ledger/runs/6032177203?check_suite_focus=true Resolved with #1018 when you have a chance to review. |
I think some of these should just be
I added #1036 to track other changes that we may want to consider but the two changes listed above might be nice to include within this pull request. |
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.
Reviewed API changes primarily and those LGTM
// of the retired credits. This must be provided if retired_amount is | ||
// positive. It is a string of the form <country-code>[-<sub-national-code>[ | ||
// <postal-code>]], with the first two fields conforming to ISO 3166-2, and | ||
// postal-code being up to 64 alphanumeric characters. |
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.
It's maybe not a good idea to duplicate all these docs
Co-authored-by: Aaron Craelius <aaron@regen.network>
It looks like the above changes are the only changes necessary needed for #1036 and would have been nice to include here. Also would have been nice to address the proto documentation suggestion aaron made. Let's make sure are addressing suggestions before merging in the future. |
Description
Closes: #1012
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