Skip to content
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 SetScalingFactorController gov prop #5937

Merged

Conversation

sampocs
Copy link
Contributor

@sampocs sampocs commented Aug 1, 2023

Closes: #XXX

What is the purpose of the change

Adds a gov prop to update the scaling factor controller address of a stableswap pool. This is particularly useful when migrating from a multisig controller to a CW contract controller.

Testing and Verifying

Setup

  • Start localosmosis
  • Modify the stableswapPool.json to include the scaling-factor-controller address
{
        "initial-deposit": "1000000stake,1000uosmo",
        "swap-fee": "0.005",
        "exit-fee": "0",
        "future-governor": "",
        "scaling-factors": "1000,1",
        "scaling-factor-controller": "osmo19e2mf7cywkv7zaug6nk5f87d07fxrdgrladvymh2gwv5crvm3vnsuewhh7"
}
  • Add controller key
echo "retire sport observe pyramid hole venue simple panel seven trophy group kiss shoe caution length farm sail main hunt client level hazard alien mirror" | osmosisd keys add controller --recover --keyring-backend test
osmosisd tx bank send osmo12smx2wdlyttvyzvzg54y2vnqwq2qjateuf7thj osmo13zfmpc7lr97ud4sg335wxt3e4x9x8zr7crvqyu 1000000uosmo --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Create a stableswap pool
osmosisd tx gamm create-pool --pool-type stableswap --pool-file stableswapPool.json \
     --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Show current address
>>> osmosisd q gamm pools | grep controller
  scaling_factor_controller: osmo19e2mf7cywkv7zaug6nk5f87d07fxrdgrladvymh2gwv5crvm3vnsuewhh7

Update controller address with proposal.json

  • Create proposal.json
{
	"title": "Set Scaling Factor Controller Proposal",
	"description": "Change scaling factor controller address",
	"pool_id": 1,
	"controller_address": "osmo1jg7w3rt85tyj2hxky206chj23ts3nxm43gll3q"
}
  • Submit proposal
osmosisd tx gov submit-proposal set-scaling-factor-controller-proposal \
    --proposal proposal.json  --deposit "10000000uosmo" \
    --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Vote on proposal
osmosisd tx gov vote 1 yes --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Wait for voting period to finish and confirm address updated
>>> osmosisd q gamm pools | grep controller
  scaling_factor_controller: osmo1jg7w3rt85tyj2hxky206chj23ts3nxm43gll3q

Update controller address with flags

  • Submit proposal with flags
osmosisd tx gov submit-proposal set-scaling-factor-controller-proposal \
      --title "Set Scaling Factor Controller Proposal" \
      --description "Change scaling factor controller address" \
      --deposit "10000000uosmo" \
      --pool-id 1 \
      --scaling-factor-controller-address "osmo13zfmpc7lr97ud4sg335wxt3e4x9x8zr7crvqyu" \
      --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Vote on proposal
osmosisd tx gov vote 2 yes --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Wait 1 minute for voting period to finish and confirm address updated
>>> osmosisd q gamm pools | grep controller
  scaling_factor_controller: osmo13zfmpc7lr97ud4sg335wxt3e4x9x8zr7crvqyu

Adjust Scaling Factor

  • Attempt to adjust the scaling factor with a non-controller address
osmosisd tx gamm adjust-scaling-factors --pool-id 1 --scaling-factors="2,1" --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Query the tx and confirm that it failed with the error not scaling factor governor
  • Attempt to adjust the scaling factor with the controller address
osmosisd tx gamm adjust-scaling-factors --pool-id 1 --scaling-factors="2,1" --from controller -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Confirm the scaling factor was updated
osmosisd q gamm pools | grep scaling_factors -A 2

Error Cases

  • Attempt to submit a proposal for a non-existent pool - query the tx and see that it failed because the pool ID does not exist
osmosisd tx gov submit-proposal set-scaling-factor-controller-proposal \
      --title "Set Scaling Factor Controller Proposal" \
      --description "Change scaling factor controller address" \
      --deposit "10000000uosmo" \
      --pool-id 100 \
      --scaling-factor-controller-address "osmo1w9e3xrjja7th9w9jje2vakghm8ppjnkrqrcm2t" \
      --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo
  • Attempt to submit a proposal with an invalid address, it should fail
osmosisd tx gov submit-proposal set-scaling-factor-controller-proposal \
      --title "Set Scaling Factor Controller Proposal" \
      --description "Change scaling factor controller address" \
      --deposit "10000000uosmo" \
      --pool-id 1 \
      --scaling-factor-controller-address "osmoXXX" \
      --from val -y --keyring-backend test --chain-id localosmosis --fees 1000uosmo

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@github-actions github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:app-wiring Changes to the app folder labels Aug 1, 2023
@czarcas7ic czarcas7ic added the V:state/breaking State machine breaking PR label Aug 1, 2023
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM! Lets get this tested on a local osmosis instance to be sure and then we can merge, thanks!

x/gamm/handler.go Outdated Show resolved Hide resolved
expError error
isStableSwapPool bool
}{
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add a test case where we try to use an invalid address as the controller addr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the address validation happens upstream in ValidateBasic. Want me to duplicate the check here as well?

@czarcas7ic
Copy link
Member

Oh and also could you please add a changelog entry as well?

Co-authored-by: Adam Tucker <adam@osmosis.team>
@sampocs
Copy link
Contributor Author

sampocs commented Aug 2, 2023

@czarcas7ic

Oh and also could you please add a changelog entry as well?

Added! (sorry forgot to push it up yesterday)

@github-actions github-actions bot added the C:CLI label Aug 2, 2023
@sampocs
Copy link
Contributor Author

sampocs commented Aug 2, 2023

Successfully tested this locally with localosmosis and added the testing flow to the PR description

@czarcas7ic

@czarcas7ic
Copy link
Member

czarcas7ic commented Aug 2, 2023

Thanks so much for adding all the CLI commands you used! Will just need a second ACK to merge but it should be a quick ACK due to all the localosmosis logs that were added.

@@ -23,6 +23,7 @@ func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
cdc.RegisterConcrete(&MsgExitSwapShareAmountIn{}, "osmosis/gamm/exit-swap-share-amount-in", nil)
cdc.RegisterConcrete(&UpdateMigrationRecordsProposal{}, "osmosis/gamm/update-migration-records-proposal", nil)
cdc.RegisterConcrete(&ReplaceMigrationRecordsProposal{}, "osmosis/gamm/replace-migration-records-proposal", nil)
cdc.RegisterConcrete(&SetScalingFactorControllerProposal{}, "osmosis/gamm/set-scaling-factor-controller-address", nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is too long to work with ledgers, cc @mattverse isn't the limit 40 characters? Or does that not apply to gov props?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I think it's actually 39 characters. I'm not sure if it applies to gov props

Either way, shortened it to be safe [here]

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Nice job! I think we need a different amino string due to the 40 char limit from the SDK. (It really should be in the API used for registration :/ , looks like some of our existing messags there have problems :/ )

@czarcas7ic czarcas7ic merged commit e5b8273 into osmosis-labs:main Aug 5, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:CLI C:x/gamm Changes, features and bugs related to the gamm module. V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants