-
Notifications
You must be signed in to change notification settings - Fork 607
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
Allow whitelisted tx fee tokens based on conversion rate to OSMO #394
Conversation
func GetCmdDenomPoolID() *cobra.Command { | ||
cmd := &cobra.Command{ | ||
Use: "denom-pool-id", | ||
Short: "Query the pool id associated with a specific whitelisted fee token", |
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.
If ATOM is whitelisted for ATOM/OSMO and ATOM/AKT pair, what does this return?
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 will check that only one pool can be whitelisted per denom. It also should make sure that selected pool does contain the native fee token (OSMO)
Going to take over this PR in a new branch, for an MVP of just the state changes & ensuring only allowed fee tokens are put on change. This would then enable tx fees in CheckTx to be written in a second PR |
b6a5f4c
to
e4ae67c
Compare
Should merge osmosis-labs/cosmos-sdk#42 first |
Thanks @antstalepresh for the great review! |
Co-authored-by: Dev Ojha <[email protected]>
PR looks good for me, testing the module with CLI command and adding README would be helpful. |
x/txfees/module.go
Outdated
func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) { | ||
types.RegisterCodec(cdc) | ||
} |
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.
Not used in https://github.com/cosmos/cosmos-sdk/blob/master/x/authz/module/module.go#L50, which I believe has ledger support.
Co-authored-by: Dev Ojha <[email protected]>
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.
New tests look great to me! Lets merge once CI passes
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
==========================================
+ Coverage 20.31% 20.72% +0.40%
==========================================
Files 164 172 +8
Lines 23262 23465 +203
==========================================
+ Hits 4726 4863 +137
- Misses 17762 17812 +50
- Partials 774 790 +16 ☔ View full report in Codecov by Sentry. |
closes #404