-
Notifications
You must be signed in to change notification settings - Fork 411
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
Use gov v1 instead of v1beta1 for new proposals #1269
Conversation
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.
Very good start. This is not a trivial task 👍
Indeed, @alpe is right, this is absolutely not a trivial task. Thanks for taking this on. |
3c1bd4e
to
a6d571b
Compare
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.
Thanks for the updates! I added some minor ones.
Looking forward to see the integration with the keeper for the gov process
if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { | ||
return errorsmod.Wrap(err, "authority") | ||
} | ||
|
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.
Use ValidateVerificationInfo(p.Source, p.Builder, p.CodeHash)
as in StoreAndInstantiateContractProposal
for optional fields
Codecov Report
@@ Coverage Diff @@
## main #1269 +/- ##
==========================================
+ Coverage 59.49% 63.47% +3.98%
==========================================
Files 57 56 -1
Lines 7320 6971 -349
==========================================
+ Hits 4355 4425 +70
+ Misses 2652 2218 -434
- Partials 313 328 +15
|
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.
Very nice work! Thanks for driving this forward. 🥇
I have added some minor nits and comments. Almost good to go 👏
}, nil | ||
} | ||
|
||
func (m msgServer) selectAuthorizationPolicy(actor string) AuthorizationPolicy { |
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.
👍
x/wasm/client/cli/tx.go
Outdated
@@ -44,6 +44,7 @@ const ( | |||
flagMaxFunds = "max-funds" | |||
flagAllowAllMsgs = "allow-all-messages" | |||
flagNoTokenTransfer = "no-token-transfer" //nolint:gosec | |||
flagAuthority = "authority" |
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 flag is unused now and can be removed
x/wasm/client/proposal_handler.go
Outdated
govclient.NewProposalHandler(cli.ProposalStoreAndInstantiateContractCmd), | ||
govclient.NewProposalHandler(cli.ProposalInstantiateContract2Cmd), | ||
} | ||
var ProposalHandlers = []govclient.ProposalHandler{} |
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.
let's get rid of this, too.
x/wasm/keeper/proposal_handler.go
Outdated
@@ -20,6 +20,8 @@ func NewWasmProposalHandler(k decoratedKeeper, enabledProposalTypes []types.Prop | |||
} | |||
|
|||
// NewWasmProposalHandlerX creates a new governance Handler for wasm proposals | |||
// | |||
//nolint:staticcheck |
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.
please add Deprecated
annotation here and NewWasmProposalHandler
to mark this
@@ -63,6 +65,7 @@ func NewWasmProposalHandlerX(k types.ContractOpsKeeper, enabledProposalTypes []t | |||
} | |||
} | |||
|
|||
//nolint:staticcheck |
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.
👍
@@ -38,6 +38,7 @@ const ( | |||
|
|||
// WasmKeeper is a subset of the wasm keeper used by simulations | |||
type WasmKeeper interface { | |||
GetAuthority() string |
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.
👍
&MsgSudoContract{}, | ||
&MsgPinCodes{}, | ||
&MsgUnpinCodes{}, | ||
&MsgStoreAndInstantiateContract{}, |
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.
👍
cdc.RegisterConcrete(&MsgSudoContract{}, "wasm/MsgSudoContract", nil) | ||
cdc.RegisterConcrete(&MsgPinCodes{}, "wasm/MsgPinCodes", nil) | ||
cdc.RegisterConcrete(&MsgUnpinCodes{}, "wasm/MsgUnpinCodes", nil) | ||
cdc.RegisterConcrete(&MsgStoreAndInstantiateContract{}, "wasm/MsgStoreAndInstantiateContract", nil) |
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.
👍 still required
} | ||
}) | ||
} | ||
} |
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.
Very nice to see all these tests! This will give high confidence that things work as expected. Thanks for taking care!
Can you add a test for UpdateInstantiateConfig
so that we have this covered, too?
@@ -60,65 +57,3 @@ func TestParseAccessConfigFlags(t *testing.T) { | |||
}) | |||
} | |||
} |
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.
Can you add tests for the new messages ValidateBasic
for regression?
- MsgUpdateParams
- MsgPinCodes
- MsgUnpinCodes
- MsgSudoContract
- MsgStoreAndInstantiateContract
Part of #1246
With this PR, the v1beta1 wasm proposal types are removed from CLI and types and methods are deprecated.
In-flight proposals can still be executed but no new ones added.
In order to make use of the new gov v1 types, you can use the new wizard
wasmd tx gov draft-proposal
to create a template.Update:
The proposal template generator is not really a good fit with for wasm proposals due to their more complex fields. We will follow up on this