feat(superfluid/gov): configure unpool whitelist via gov prop #3512

p0mvn · 2022-11-24T21:26:17Z

Closes: #XXX

What is the purpose of the change

Add the ability to configure the unpool whitelist via gov prop.

The new gov prop takes the list of pool ids and isOverwriteFlag. If the flag is true, the old pool ids are discarded. If false, the old pool ids are merged with the new.

Brief Changelog

wired up gov prop boilerplate
implemented validation in the handler
unit tested
Took store helper and tests from CL branch:
- Add state for Mint: tick and Position #3136
- add tests for GetIfFound #3145

Testing and Verifying

uni tests added
localnet test

# Create 3 pools by running the below 3 times
osmosisd tx gamm create-pool --pool-file=tests/e2e/scripts/nativeDenomPool.json  --from lo-test1 --keyring-backend test -b=block

# Fails because pool id 4 does not exist
osmosisd tx gov submit-proposal update-unpool-whitelist --pool-ids "4" --title "test" --description "test"  --from lo-test1 --keyring-backend test -b=block

# Succesfully creates proposal
osmosisd tx gov submit-proposal update-unpool-whitelist --pool-ids "1, 2, 3" --title "test" --description "test"  --from lo-test1 --keyring-backend test -b=block

Upon enabling whitelist, it is still impossible to run:

# Fails with message not activated.
 osmosisd tx superfluid unpool-whitelisted-pool 1 --from lo-test1 --keyring-backend test -b=bloc

We should enable that message in a separate PR.

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? godoc

x/superfluid/keeper/gov/suite_test.go

* add tests for GetIfFound * refactor

ValarDragon · 2022-11-25T19:11:10Z

osmoutils/store_helper.go

+
+// GetIfFound gets key from store
+// returns a boolean indicating whether value exists for the given key and error
+func GetIfFound(store store.KVStore, key []byte, result proto.Message) (found bool, err error) {


Can be changed later / can discuss in an issue, should not block this PR. Perhaps this should be called HasWithType ?

e.g. if we were masters of the store interface

store.HasProtoMsg("myKey", types.Pool)

Has to indicate its a bool (rather than assuming you'd get the object back)

OR is proto.Message here actually mutated? If so lets update the comment, but this doesn't actually make sense to me as an API.

Why don't just make a method called Get thats the same as MustGet, but returns an error instead of panics?

ValarDragon · 2022-11-25T19:12:20Z

x/superfluid/client/rest/tx.go

@@ -10,23 +10,25 @@ import (
 func ProposalSetSuperfluidAssetsRESTHandler(clientCtx client.Context) govrest.ProposalRESTHandler {
 	return govrest.ProposalRESTHandler{
 		SubRoute: "set-superfluid-assets",
-		Handler:  newSetSuperfluidAssetsHandler(clientCtx),


idk whats going on here, but IIRC this REST folder doesn't matter for anything / should be deleted, so not blocking

For each of these, the handler is the same - no-op. I just made a shared no-op function for all of these but yes it's unimportant

ah that makes sense! Nice :)

x/superfluid/types/gov.go

ValarDragon

Looks great to me! Can we just sanity check that we have an existing test for ensuring that unpooling doesn't work for pools not in whitelist?

ValarDragon · 2022-11-25T19:48:13Z

x/superfluid/keeper/gov/gov_test.go

+func (suite *KeeperTestSuite) TestHandleUnpoolWhiteListChange() {
+	const (
+		testTitle       = "test title"
+		testDescription = "test description"
+	)


Great job on the test!

Co-authored-by: Dev Ojha <[email protected]>

czarcas7ic

Well done 👍

czarcas7ic · 2022-11-26T01:13:25Z

x/superfluid/client/cli/tx.go

+		Args:  cobra.ExactArgs(0),
+		Short: "Update unpool whitelist proposal",
+		Long: "This proposal will update the unpool whitelist if passed. " +
+			"Every pool id must be valid. If proposal passes, and the pool id is invalid, the proposal will not take any effect. " +


Why don't we run GetPoolAndPoke in ValidateBasic to prevent this from happening?

Unfortunately, we don't have access to the gamm keeper in ValidateBasic().

However, I found out that gov module attempts to test-run the proposal with cache context before allowing it to be submitted:
https://github.com/osmosis-labs/cosmos-sdk/blob/870132d311d162fdbe0e92fcc0a42ee3889afd0d/x/gov/keeper/proposal.go#L22-L29

So if pool doesn't exist, it will not be submitted. I will change the wording

* feat(superfluid/gov): configure unpool whitelist via gov prop * cli and amino registrateions * linter * Update x/superfluid/keeper/gov/suite_test.go * Update x/superfluid/keeper/gov/suite_test.go * add tests for GetIfFound (#3145) * add tests for GetIfFound * refactor * cli fix * cli updates * remove message not activated * Update x/superfluid/types/gov.go Co-authored-by: Dev Ojha <[email protected]> * correct CLI wording * GetIfFound godoc Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Dev Ojha <[email protected]> (cherry picked from commit 404cf9b)

p0mvn · 2022-11-26T02:57:36Z

Looks great to me! Can we just sanity check that we have an existing test for ensuring that unpooling doesn't work for pools not in whitelist?

Test added: #3532

…#3531) * feat(superfluid/gov): configure unpool whitelist via gov prop * cli and amino registrateions * linter * Update x/superfluid/keeper/gov/suite_test.go * Update x/superfluid/keeper/gov/suite_test.go * add tests for GetIfFound (#3145) * add tests for GetIfFound * refactor * cli fix * cli updates * remove message not activated * Update x/superfluid/types/gov.go Co-authored-by: Dev Ojha <[email protected]> * correct CLI wording * GetIfFound godoc Co-authored-by: Hieu Vu <[email protected]> Co-authored-by: Dev Ojha <[email protected]> (cherry picked from commit 404cf9b) Co-authored-by: Roman <[email protected]>

github-actions bot added C:app-wiring Changes to the app folder C:x/superfluid labels Nov 24, 2022

p0mvn force-pushed the roman/unpool branch from 73d6c18 to ec5d83c Compare November 24, 2022 21:27

feat(superfluid/gov): configure unpool whitelist via gov prop

4460a80

p0mvn force-pushed the roman/unpool branch from ec5d83c to 4460a80 Compare November 24, 2022 21:37

p0mvn added V:state/compatible/no_backport State machine compatible PR, depends on prior breaks V:state/compatible/backport State machine compatible PR, should be backported and removed V:state/compatible/no_backport State machine compatible PR, depends on prior breaks labels Nov 24, 2022

cli and amino registrateions

abb8b8b

github-actions bot added the C:CLI label Nov 24, 2022

linter

611e273

p0mvn marked this pull request as ready for review November 25, 2022 02:19

p0mvn requested a review from a team November 25, 2022 02:19

p0mvn commented Nov 25, 2022

View reviewed changes

x/superfluid/keeper/gov/suite_test.go Outdated Show resolved Hide resolved

x/superfluid/keeper/gov/suite_test.go Outdated Show resolved Hide resolved

p0mvn and others added 5 commits November 24, 2022 22:54

Update x/superfluid/keeper/gov/suite_test.go

680e4fd

Update x/superfluid/keeper/gov/suite_test.go

06bb0c3

Merge branch 'main' into roman/unpool

861cf9d

add tests for GetIfFound (#3145)

8c046f8

* add tests for GetIfFound * refactor

cli fix

a4aa501

p0mvn added V:state/breaking State machine breaking PR A:backport/v13.x backport patches to v13.x branch and removed V:state/compatible/backport State machine compatible PR, should be backported labels Nov 25, 2022

cli updates

ca2412e

ValarDragon reviewed Nov 25, 2022

View reviewed changes

remove message not activated

f00782f

ValarDragon reviewed Nov 25, 2022

View reviewed changes

x/superfluid/types/gov.go Outdated Show resolved Hide resolved

ValarDragon approved these changes Nov 25, 2022

View reviewed changes

ValarDragon reviewed Nov 25, 2022

View reviewed changes

Update x/superfluid/types/gov.go

0e700d7

Co-authored-by: Dev Ojha <[email protected]>

czarcas7ic approved these changes Nov 26, 2022

View reviewed changes

correct CLI wording

e4c0ee2

p0mvn mentioned this pull request Nov 26, 2022

osmoutils.GetIfFound semantics discussion #3530

Closed

GetIfFound godoc

0a116d4

p0mvn merged commit 404cf9b into main Nov 26, 2022

p0mvn deleted the roman/unpool branch November 26, 2022 02:55

mergify bot mentioned this pull request Nov 26, 2022

feat(superfluid/gov): configure unpool whitelist via gov prop (backport #3512) #3531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(superfluid/gov): configure unpool whitelist via gov prop #3512

feat(superfluid/gov): configure unpool whitelist via gov prop #3512

p0mvn commented Nov 24, 2022 •

edited

Loading

ValarDragon Nov 25, 2022

ValarDragon Nov 25, 2022 •

edited

Loading

p0mvn Nov 25, 2022

ValarDragon Nov 25, 2022 •

edited

Loading

ValarDragon left a comment

ValarDragon Nov 25, 2022

czarcas7ic left a comment

czarcas7ic Nov 26, 2022

p0mvn Nov 26, 2022

p0mvn commented Nov 26, 2022

feat(superfluid/gov): configure unpool whitelist via gov prop #3512

feat(superfluid/gov): configure unpool whitelist via gov prop #3512

Conversation

p0mvn commented Nov 24, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

ValarDragon Nov 25, 2022

Choose a reason for hiding this comment

ValarDragon Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

p0mvn Nov 25, 2022

Choose a reason for hiding this comment

ValarDragon Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

ValarDragon Nov 25, 2022

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

czarcas7ic Nov 26, 2022

Choose a reason for hiding this comment

p0mvn Nov 26, 2022

Choose a reason for hiding this comment

p0mvn commented Nov 26, 2022

p0mvn commented Nov 24, 2022 •

edited

Loading

ValarDragon Nov 25, 2022 •

edited

Loading

ValarDragon Nov 25, 2022 •

edited

Loading