osmosis icon indicating copy to clipboard operation
osmosis copied to clipboard

modify pool-incentives submit proposal cli

Open catShaark opened this issue 2 years ago • 3 comments

Closes: #1888

Brief Changelog

  • add proposal-specific flags to NewCmdSubmitUpdatePoolIncentivesProposal and NewCmdSubmitReplacePoolIncentivesProposal
  • disable poolincetives tx commands since these should be only used with a gov prefix
  • register NewCmdSubmitReplacePoolIncentivesProposal handler on the gov module

Testing and Verifying

  • Manually verified

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? not documented

catShaark avatar Jul 15 '22 18:07 catShaark

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 https://github.com/osmosis-labs/osmosis/blob/fd23552f94caf1df9dbeb6fb4784e9d0efded2a4/x/pool-incentives/handler.go#L12-L24

So the routes being equivalent may be correct?

ValarDragon avatar Aug 04 '22 22:08 ValarDragon

(this is also needlessly complex, and we should simplify it)

ValarDragon avatar Aug 04 '22 22:08 ValarDragon

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

https://github.com/osmosis-labs/osmosis/blob/fd23552f94caf1df9dbeb6fb4784e9d0efded2a4/x/pool-incentives/handler.go#L12-L24

So the routes being equivalent may be correct?

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:

  • osmosisd tx poolincentives update-pool-incentives
  • osmosisd tx gov submit-proposal update-pool-incentives

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 ProposalType() is used in routing does seem to be correct. Although this is client-side (rest) routing.

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

p0mvn avatar Aug 05 '22 02:08 p0mvn

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!

github-actions[bot] avatar Aug 20 '22 00:08 github-actions[bot]

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

p0mvn avatar Aug 23 '22 01:08 p0mvn

I think it would be great to have an e2e test for submitting these pool-incentive proposals. Would you be interested in adding it?

p0mvn avatar Aug 23 '22 01:08 p0mvn