Add UpdateInstantiateConfig gov proposal #796

jhernandezb · 2022-04-01T15:23:04Z

closes #780
closes #810

Server Side
Client Side
Testing

jhernandezb · 2022-04-01T15:24:44Z

@alpe Regarding single update vs multiple updates on a single proposal. Should it be one proposal at a time? or udpating multiple is fine? also different configs? Currently I have multiple code ids with the same AccessConfig.

codecov · 2022-04-01T15:32:36Z

Codecov Report

Merging #796 (22144c3) into main (5bb0673) will decrease coverage by 1.16%.
The diff coverage is 14.37%.

@@            Coverage Diff             @@
##             main     #796      +/-   ##
==========================================
- Coverage   59.89%   58.72%   -1.17%     
==========================================
  Files          52       52              
  Lines        5987     6140     +153     
==========================================
+ Hits         3586     3606      +20     
- Misses       2134     2266     +132     
- Partials      267      268       +1

Impacted Files	Coverage Δ
x/wasm/client/cli/gov_tx.go	`0.00% <0.00%> (ø)`
x/wasm/client/cli/query.go	`0.00% <0.00%> (ø)`
x/wasm/types/proposal.go	`63.12% <6.25%> (-5.93%)`	⬇️
x/wasm/keeper/contract_keeper.go	`92.85% <100.00%> (+0.54%)`	⬆️
x/wasm/keeper/keeper.go	`87.88% <100.00%> (-0.21%)`	⬇️
x/wasm/keeper/proposal_handler.go	`66.44% <100.00%> (+2.15%)`	⬆️
x/wasm/types/codec.go	`100.00% <100.00%> (ø)`

alpe · 2022-04-04T13:19:05Z

@jhernandezb good start! You are more familiar with the concrete use case but from my perspective multiple code_ids with same AccessConfig make a lot of sense. This can be explained to the voters but also gives flexibility to address multiple code ids.

jhernandezb · 2022-04-04T18:54:42Z

Yes my reasoning is for example: Freeze Code ID 1,2,3,4 then you only need to apply the same config to all of them. If someone wanted different access configs then a different proposal should be made explaining the reasoning.

Will complete this PR then and push updates

ethanfrey · 2022-04-25T09:47:31Z

This will close #780 right?

If so, please add that to the description

ethanfrey · 2022-05-04T14:56:31Z

@alpe Regarding single update vs multiple updates on a single proposal. Should it be one proposal at a time? or udpating multiple is fine? also different configs? Currently I have multiple code ids with the same AccessConfig.

I would either:

One code Id, one instantiate config
A list of N repeated (code_id, instantiate_config) submessages

I have no strong opinion either way. Although I guess the second would be better to match the use case you had in mind to allow multiple code_ids

ethanfrey · 2022-05-04T20:13:18Z

If we can get this ready by (early?) next week, I would love to get it in the upcoming v0.27 release

Please let us know when it is ready for another review and if that seems feasible for you.

jhernandezb · 2022-05-04T20:15:23Z

working on this today can have a PR ready for review tomorrow so I think its doable to have it for v0.27

jhernandezb · 2022-05-05T19:19:40Z

@ethanfrey @alpe this is ready for review will be doing some manual testing.

Some notes:

Added a new method to the keeper SetAccessConfig
added testing to proposal_integration tests with different cases but let me know if it can be improved
Moved to A list of N repeated (code_id, instantiate_config) submessages approach
CLI command is a bit tricky with that approach but added examples

alpe

Excellent work. I am very happy with this PR.

Only some comments on rest that need be addressed before merging.

Too bad we have not dropped legacy rest support already.

Makefile

x/wasm/client/cli/gov_tx.go

x/wasm/client/cli/query.go

x/wasm/client/rest/gov.go

x/wasm/types/proposal.go

x/wasm/keeper/proposal_integration_test.go

x/wasm/types/proposal.go

x/wasm/keeper/proposal_integration_test.go

jhernandezb · 2022-05-06T15:35:10Z

Made some updates just have couple questions
#796 (comment)
#796 (comment)

ethanfrey · 2022-05-09T13:46:27Z

@alpe does this address all your concerns?

I will do a review and merge this evening / tomorrow unless there is a blocker

alpe

Very nice 👍

ethanfrey

lgtm. I would love to see a manual test (as there are all kinds of codecs and middleware called in the real proposal stack), but good stuff.

I added a few comments on naming, which would be nice polish.
A CHANGELOG entry (one line pointing to this PR) would also be nice.

Happy to merge once those are done (just mention me in a comment and I will double check)

ethanfrey · 2022-05-09T15:09:34Z

proto/cosmwasm/wasm/v1/proposal.proto

+  // CodeID is the reference to the stored WASM code to be updated
+  uint64 code_id = 1 [ (gogoproto.customname) = "CodeID" ];
+  // InstantiatePermission to apply to the set of code ids
+  AccessConfig instantiate_permission = 4 [ (gogoproto.nullable) = false ];


Odd that this is 4. Not wrong, just surprising, I assume this was for some backwards compatibility with another struct?

It would be good to label it if so, with something like "2 and 3 are reserved" or such

it should be 2 will fix.

ethanfrey · 2022-05-09T15:11:38Z

x/wasm/client/cli/gov_tx.go

+	cmd.Flags().String(cli.FlagDeposit, "", "Deposit of proposal")
+	cmd.Flags().String(cli.FlagProposal, "", "Proposal file path (if this path is given, other proposal flags are ignored)")
+	// type values must match the "ProposalHandler" "routes" in cli
+	cmd.Flags().String(flagProposalType, "", "Permission of proposal, types: store-code/instantiate/migrate/update-admin/clear-admin/text/parameter_change/software_upgrade")


I'm a bit confused why this flag is here?

Usage is tx gov submit-proposal update-instantiate-config 1,nobody 2,everybody 3,%s1l2rsakp388kuv9k8qzq6lrm9taddae7fpx59wm right?

this is unrelated to this logic, but it's used by this https://github.com/cosmos/cosmos-sdk/blob/v0.45.4/x/gov/client/cli/parse.go#L13

All other proposals have it but I don't think its actually used and could be removed.

Good point, can you make a follow up (cleanup issue), so someone can look into that later.
Not blocking merge at all, but don't want to forget it

ethanfrey · 2022-05-09T15:13:23Z

x/wasm/client/rest/gov.go

+	Proposer    string    `json:"proposer" yaml:"proposer"`
+	Deposit     sdk.Coins `json:"deposit" yaml:"deposit"`
+
+	CodeUpdates []types.CodeAccessConfigUpdate `json:"code_updates" yaml:"code_updates"`


I think access_config_updates or code_access_config_updates would work (and ideally update both in the Protobuf file as well)

ethanfrey · 2022-05-09T15:16:00Z

x/wasm/keeper/proposal_integration_test.go

@@ -749,3 +749,98 @@ func TestUnpinCodesProposal(t *testing.T) {
 		})
 	}
 }
+
+func TestUpdateInstantiateConfigProposal(t *testing.T) {


ethanfrey · 2022-05-09T15:17:25Z

x/wasm/types/exported_keepers.go

@@ -53,6 +53,9 @@ type ContractOpsKeeper interface {

 	// SetContractInfoExtension updates the extension point data that is stored with the contract info
 	SetContractInfoExtension(ctx sdk.Context, contract sdk.AccAddress, extra ContractInfoExtension) error
+
+	// SetAccessConfig updates the access config of a code id.
+	SetAccessConfig(ctx sdk.Context, codeID uint64, config AccessConfig) error


good point here. If there is a simple way to keep it private and expose to the proposal handler, maybe @alpe can make a quick PR to show how. Otherwise, I would be fine to merge like this.

jhernandezb · 2022-05-09T18:09:45Z

@ethanfrey addressed your comments and also here is a demo
https://www.loom.com/share/e99721a095d4428d80f81496daaee2ef
(ignore the print log on submit proposal, I had that as debug on my local cosmos-sdk dependency)

ethanfrey

Great stuff and loving the loom video!

Thank you very much

jhernandezb force-pushed the jhernandezb/update-instantiate-config-proposal branch from 1564a54 to 582f435 Compare April 2, 2022 01:23

jhernandezb force-pushed the jhernandezb/update-instantiate-config-proposal branch from 582f435 to e43b811 Compare April 11, 2022 05:00

ethanfrey added this to the v0.27.0 milestone Apr 25, 2022

ethanfrey added the optional Not absolutely required for the milestone label Apr 25, 2022

jhernandezb force-pushed the jhernandezb/update-instantiate-config-proposal branch from e43b811 to fde825b Compare May 3, 2022 15:12

jhernandezb force-pushed the jhernandezb/update-instantiate-config-proposal branch from fde825b to 3cc80d8 Compare May 4, 2022 18:31

This was referenced May 4, 2022

Simpler upload/instantiate flow for permissioned chains #840

Closed

StoreAndInstantiate gov proposal #785

Closed

jhernandezb changed the title ~~[WIP] Add UpdateInstantiateConfig gov proposal~~ Add UpdateInstantiateConfig gov proposal May 5, 2022

jhernandezb marked this pull request as ready for review May 5, 2022 19:14

jhernandezb requested a review from alpe as a code owner May 5, 2022 19:14

jhernandezb force-pushed the jhernandezb/update-instantiate-config-proposal branch from 3e4619a to 8c78632 Compare May 6, 2022 03:28

alpe mentioned this pull request May 6, 2022

Update instantiate config command #843

Closed

alpe suggested changes May 6, 2022

View reviewed changes

jhernandezb force-pushed the jhernandezb/update-instantiate-config-proposal branch from 753b9c2 to 242bb3c Compare May 6, 2022 15:39

alpe approved these changes May 9, 2022

View reviewed changes

ethanfrey reviewed May 9, 2022

View reviewed changes

jhernandezb added 2 commits May 9, 2022 11:35

add UpdateInstantiateConfig gov proposal

72710ef

fix Makefile

e490a5e

jhernandezb added 10 commits May 9, 2022 11:35

update UpdateInstantiateConfigProposal proto definition

cd76a61

add to EnableAllProposals

7c0763e

add tests

01157fa

add to docs

931bffe

Add handlers

bf9c6e2

add code-info query command

86c61ef

fix missing gov registration type

a890832

address code review comments

30f0572

fix error message

ac6876f

update proto field name

3ade815

jhernandezb force-pushed the jhernandezb/update-instantiate-config-proposal branch from 242bb3c to 3ade815 Compare May 9, 2022 17:35

add to CHANGELOG

22144c3

ethanfrey approved these changes May 9, 2022

View reviewed changes

ethanfrey merged commit 5233ed7 into CosmWasm:main May 9, 2022

jhernandezb mentioned this pull request May 9, 2022

Remove unused flag in gov proposals #849

Closed

ethanfrey mentioned this pull request May 9, 2022

Enforce default code permissions #850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UpdateInstantiateConfig gov proposal #796

Add UpdateInstantiateConfig gov proposal #796

jhernandezb commented Apr 1, 2022 •

edited

Loading

jhernandezb commented Apr 1, 2022 •

edited

Loading

codecov bot commented Apr 1, 2022 •

edited

Loading

alpe commented Apr 4, 2022

jhernandezb commented Apr 4, 2022

ethanfrey commented Apr 25, 2022

ethanfrey commented May 4, 2022

ethanfrey commented May 4, 2022 •

edited

Loading

jhernandezb commented May 4, 2022

jhernandezb commented May 5, 2022

alpe left a comment

jhernandezb commented May 6, 2022

ethanfrey commented May 9, 2022

alpe left a comment

ethanfrey left a comment •

edited

Loading

ethanfrey May 9, 2022

jhernandezb May 9, 2022

ethanfrey May 9, 2022

jhernandezb May 9, 2022

ethanfrey May 9, 2022

ethanfrey May 9, 2022

ethanfrey May 9, 2022

ethanfrey May 9, 2022

jhernandezb commented May 9, 2022 •

edited

Loading

ethanfrey left a comment

Add UpdateInstantiateConfig gov proposal #796

Add UpdateInstantiateConfig gov proposal #796

Conversation

jhernandezb commented Apr 1, 2022 • edited Loading

jhernandezb commented Apr 1, 2022 • edited Loading

codecov bot commented Apr 1, 2022 • edited Loading

Codecov Report

alpe commented Apr 4, 2022

jhernandezb commented Apr 4, 2022

ethanfrey commented Apr 25, 2022

ethanfrey commented May 4, 2022

ethanfrey commented May 4, 2022 • edited Loading

jhernandezb commented May 4, 2022

jhernandezb commented May 5, 2022

alpe left a comment

Choose a reason for hiding this comment

jhernandezb commented May 6, 2022

ethanfrey commented May 9, 2022

alpe left a comment

Choose a reason for hiding this comment

ethanfrey left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhernandezb commented May 9, 2022 • edited Loading

ethanfrey left a comment

Choose a reason for hiding this comment

jhernandezb commented Apr 1, 2022 •

edited

Loading

jhernandezb commented Apr 1, 2022 •

edited

Loading

codecov bot commented Apr 1, 2022 •

edited

Loading

ethanfrey commented May 4, 2022 •

edited

Loading

ethanfrey left a comment •

edited

Loading

jhernandezb commented May 9, 2022 •

edited

Loading