-
Notifications
You must be signed in to change notification settings - Fork 593
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(transfer)!: migrate transfer parameters to be self managed #3553
Conversation
…ms' function and update tests
…nt the custom logic
… functions need to be updated
…ey should be managed now
…bled' in 'relay.go'
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3553 +/- ##
==========================================
+ Coverage 78.75% 78.79% +0.04%
==========================================
Files 186 186
Lines 12744 12773 +29
==========================================
+ Hits 10036 10064 +28
- Misses 2276 2278 +2
+ Partials 432 431 -1
|
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.
Last few nits from me, looking really good!
…nage-transfer-params
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 @srdtrk, nice work! LGTM
e2e/tests/transfer/base_test.go
Outdated
var transferSelfParamsFeatureReleases = semverutil.FeatureReleases{ | ||
MajorVersion: "v8", | ||
} |
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.
feature release matrices are being grouped together in test values in #3558, just noting this for resolving merge conflicts
…nage-transfer-params
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 job with this! Just a few small comments but nothing major LGTM
// Transfer defines an rpc handler method for MsgTransfer. | ||
func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.MsgTransferResponse, error) { | ||
ctx := sdk.UnwrapSDKContext(goCtx) | ||
|
||
if !k.GetSendEnabled(ctx) { | ||
if !k.GetParams(ctx).SendEnabled { |
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 think I prefer just accessing the fields directly from params rather than a getter which would include no additional logic. I don't think params needs to be anything more than just a struct of values.
Description
With this PR:
transfer
app now self-manages its parameters.transfer
app's parameters.x/params
' subspace to thetransfer
app's keeper.closes: #3502
addresses: #2010
Changes:
MsgUpdateParams
, has been introduced, executed with an authority that is expected to be thex/gov
module account.Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.