-
Notifications
You must be signed in to change notification settings - Fork 170
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: add UpdateRegistry
msg for gov proposal to update the token registry
#1486
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1486 +/- ##
==========================================
+ Coverage 52.47% 55.33% +2.86%
==========================================
Files 73 66 -7
Lines 7040 6636 -404
==========================================
- Hits 3694 3672 -22
+ Misses 3073 2691 -382
Partials 273 273
|
@robert-zaremba should we remove |
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.
Could we make x/leverage follow the pattern seen elsewhere in the sdk, with
// GetAuthority returns the x/leverage module's authority.
func (k Keeper) GetAuthority() string {
return k.authority
}
with k.authority
set to authtypes.NewModuleAddress(govtypes.ModuleName).String()
?
Similarly, MsgUpdateRegistry
field Creator
can be renamed to Authority
and any mismatch should be rejected with err "expected gov account as only signer for proposal message"
See x/bank example
The old proposal type can be removed in this PR, since we're adding the new one. |
Ok |
I updated the changes with |
Yes |
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 so far - would it be possible to create an example proposal to add to documentation?
Note: we should delay merging until after 3.1.0 release.
Removing blocker, but don't merge until after 3.1.0
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.
I've made a suggestion to change the message behavior a bit. Also, let's put example proposal file in docs. This information should not be in infra
because it should be available to the community.
@toteki I removed remove functionality and updated |
If you could
then I think we can merge this. Edit: There's also a failing test - needs the field |
@robert-zaremba @toteki now I moved addTokens to MsgAddTokensToRegistry message from updateRegistry , so now updateRegistry only do update the token setttings and addTokensRegistry only add tokens to registry |
Following this comment on a separate PR, could we rename
|
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.
Almost there. Can we query an address of the gov account, rather than recreating it? (see comments)
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.
Ran proto format to remove some extra whitespace. LGTM
Description
closes: #1001
This pull request contains below changes:
Sample proposal.json for submit the proposal from gov
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...