-
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
modify pool-incentives submit proposal cli #2086
modify pool-incentives submit proposal cli #2086
Conversation
func (p *ReplacePoolIncentivesProposal) ProposalType() string { | ||
return ProposalTypeUpdatePoolIncentives | ||
return ProposalTypeReplacePoolIncentives | ||
} |
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.
ooof, would this be state breaking to backport? (I don't have a sense for where ProposalType gets used)
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 they're used in routing
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 used in x/gov routing when executing the proposal's handler.
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 it's only used for routing, I assume it's not going to be state-breaking to backport?
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 If we have a ProposalTypeReplacePoolIncentives submited, this will be state-breaking
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 - I think we should create a changelog entry for this
func (p *ReplacePoolIncentivesProposal) ProposalType() string { | ||
return ProposalTypeUpdatePoolIncentives | ||
return ProposalTypeReplacePoolIncentives | ||
} |
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 it's only used for routing, I assume it's not going to be state-breaking to backport?
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" | ||
|
||
"github.com/osmosis-labs/osmosis/v7/osmoutils" | ||
"github.com/osmosis-labs/osmosis/v7/x/pool-incentives/types" | ||
) | ||
|
||
func NewTxCmd() *cobra.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.
hrmm is it safe to delete this / is there a reason behind deleting this?
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 is decided in #1888 that submit proposal cmd should only have gov prefix, so these command will be registered with gov handler instead of tx cmd of incentives module.
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 - I think we should create a changelog entry for this
Should I add this to state breaking entry or not
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.
imo should go under breaking changes
Co-authored-by: Roman <[email protected]>
Sorry, I'm actually generally confused about whats going on in these governance proposal routing. My understanding is that in the today code, replace incentives proposal works on mainnet. Looking into this, looks like the handler is registered here? https://github.com/osmosis-labs/osmosis/blob/main/app/keepers/keepers.go#L366 Which then splits execution based on type osmosis/x/pool-incentives/handler.go Lines 12 to 24 in fd23552
So the routes being equivalent may be correct? |
(this is also needlessly complex, and we should simplify it) |
I think that the linked handler registration is focused on routing. On the other hand, this PR is primarily about fixing the client bootstrapping / cli. Currently, pool-incentives gov cli is broken due to incorrectly setting up the cli logic and registering some flags twice. I've verified that this should help fix the original problem in #1888. We expose 2 ways to submit pool-incentives proposals:
Only the latter is correct because it properly gets registered on the gov module. The original bug is stemming from attempting to support both ways and, as a result, registering some CLI flags twice which breaks both commands. The change related to the proposal type seems to be a pass-through change. However, It does seem like a valid change that we should keep because we were returning an invalid proposal type. The claim that Based on my investigation, the changes in this PR do not affect the state-breaking routing we are concerned about. I think we need the changes in this PR to fix the CLI and client (REST) routing. However, it might make sense to spend more time manually testing to ensure that everything is sound |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
Hi @catShaark . I just would like to confirm that you've gotten a chance to manually test that both pool incentives proposals are functioning as expected. Please let me know |
I think it would be great to have an e2e test for submitting these pool-incentive proposals. Would you be interested in adding it? |
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.
utACK. Changes lgtm.
I would like to know whether a manual test has been done or if there is intention to add an e2e test for this?
Either is fine with me to merge this
Closes: #1888
Brief Changelog
Testing and Verifying
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? yes