-
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
feat: Provide source, builder and codehash information in store code proposal message #1072
Conversation
x/wasm/types/proposal.pb.go
Outdated
} | ||
|
||
func (m *StoreCodeProposal) Reset() { *m = StoreCodeProposal{} } | ||
func (*StoreCodeProposal) ProtoMessage() {} | ||
func (*StoreCodeProposal) Descriptor() ([]byte, []int) { | ||
return fileDescriptor_be6422d717c730cb, []int{0} | ||
} | ||
|
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.
I've used make proto-gen
to generate proto code. All proto files had these formatting changes, reverted them.
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 use make proto-all
which also runs the formatter (make format
)
Codecov Report
@@ Coverage Diff @@
## main #1072 +/- ##
==========================================
+ Coverage 59.80% 59.94% +0.14%
==========================================
Files 54 54
Lines 7306 7373 +67
==========================================
+ Hits 4369 4420 +51
- Misses 2621 2632 +11
- Partials 316 321 +5
|
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 bringing this topic up and this very nice PR 💐
I think validators have a hard time to verify a StoreCodeProposal
currently. Providing additional data for this process can help with to build better tooling on the client side.
Nevertheless the data needs to be specified well and be verified, so that the expectations for everybody are the same.
We also need to support a transition period (on the server side) where fields are empty to not break execution of existing in flight proposals.
x/wasm/keeper/proposal_handler.go
Outdated
if err != nil { | ||
return err | ||
} | ||
|
||
if !bytes.Equal(checksum, p.CodeHash) { |
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.
There may be in flight proposals without this data. It would be good to not enforce the check when p.CodeHash is empty
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.
I think you got me wrong. if !bytes.Equal(checksum, p.CodeHash)
makes a lot of sense and ensures consistency of the proposal. Please add it back but with a if p.CodeHash != nil && !bytes.Equal(checksum, p.CodeHash)
so that it works for old in flight proposals (without the new fields) , too.
x/wasm/types/proposal.go
Outdated
@@ -113,6 +113,16 @@ func (p StoreCodeProposal) ValidateBasic() error { | |||
return sdkerrors.Wrap(err, "instantiate permission") | |||
} | |||
} | |||
|
|||
if len(p.Source) == 0 { |
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.
As mentioned above, these fields should be optional on the server side to support a transition period for in flight proposals.
But there is no problem to enforce checks on the CLI before submitting a proposal.
There can be stronger constraints on the expected data (when set).
x/wasm/client/cli/tx_test.go
Outdated
args: []string{"--source=bar", "--builder=foo"}, | ||
expErr: true, | ||
}, | ||
"happy path": { |
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.
I am getting an error here that I could not solve.
The hash generated on 76th line and in parseStoreCodeArgs
do not match. I decided to not get stuck and ask help from your elven 👀 @alpe here.
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.
I have commented on parseStoreCodeArgs
which contains a 🐛
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 but I think updating the TX and gen-messages should be discussed first, given the negative experience on chain in the past. Let's have an issue for them and fully focus on the gov store proposal here to get it into v0.30 if possible
proto/cosmwasm/wasm/v1/tx.proto
Outdated
@@ -41,7 +41,16 @@ message MsgStoreCode { | |||
// InstantiatePermission access control to apply on contract creation, | |||
// optional | |||
AccessConfig instantiate_permission = 5; | |||
// Source is a valid absolute HTTPS URI to the contract's source code, |
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.
The original intend as I understood it, was to add this metadata to the gov process where it made sense for me. Not sure about the TX though as they were not used proper in the past. Let's focus on the gov process in this PR and open a ticket for the TX to discuss, if you like.
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.
Makes sense 👍
x/wasm/client/cli/tx_test.go
Outdated
args: []string{"--source=bar", "--builder=foo"}, | ||
expErr: true, | ||
}, | ||
"happy path": { |
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.
I have commented on parseStoreCodeArgs
which contains a 🐛
x/wasm/client/cli/tx_test.go
Outdated
"happy path": { | ||
args: []string{"--source=bar", "--builder=foo", "--code-hash=" + checksumStr}, | ||
expErr: false, | ||
}, |
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 a case where the source file is zipped so that you are not running into a problem when comparing the hash
fyi: @pinosu is working on the StoreAndInstantiateProposal that would also benefit from the new fields |
@alpe I got carried away with other tasks, I will finish this today/tomorrow thanks! |
50904e6
to
b102cfa
Compare
b102cfa
to
477e8d9
Compare
477e8d9
to
151d18e
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. Good progress! 🤾♂️
As I had mentioned before, I would like to have some good data validation on the new fields (when set) so that we ensure they won't break clients easily.
Also the StoreAndInstantiateContractProposal
was merged to main meanwhile. This proposal would require the fields as well
Co-authored-by: Alexander Peters <[email protected]>
Bumps [github.com/cosmos/gogoproto](https://github.com/cosmos/gogoproto) from 1.4.2 to 1.4.3. - [Release notes](https://github.com/cosmos/gogoproto/releases) - [Changelog](https://github.com/cosmos/gogoproto/blob/main/CHANGELOG.md) - [Commits](cosmos/gogoproto@v1.4.2...v1.4.3) --- updated-dependencies: - dependency-name: github.com/cosmos/gogoproto dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]>
* Enable larger wasm bytecode upload for gov proposals * Set max proposal wasm code size to 3MB
* bump go 1.19 * add change log * correct change log
Maybe best to wrap source, builder and code hash information under |
@Mergifyio rebase |
* main: Upgrade to IBC v4.2.0 with interchain-accounts v0.2.4 (#1088)
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
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 work! Thanks for applying all my requests. 🥇
I am looking forward to see some client side tooling that makes use of the new fields. Great innovation 👍
// if any set require others to be set | ||
if len(source) != 0 || len(builder) != 0 || codeHash != nil { | ||
if source == "" { | ||
return fmt.Errorf("source is 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.
nit:
return fmt.Errorf("source is required") | |
return ErrEmpty.Wrap("source:) |
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.
The sdk package comes with some nice error support. Next time, please use them instead
…proposal message (CosmWasm#1072) * Provide source, builder and codehash information in store code proposal message * Make linter happy * Update x/wasm/simulation/proposals.go Co-authored-by: Alexander Peters <[email protected]> * Update x/wasm/client/cli/gov_tx.go * Update x/wasm/client/cli/gov_tx.go * Bump github.com/cosmos/gogoproto from 1.4.2 to 1.4.3 Bumps [github.com/cosmos/gogoproto](https://github.com/cosmos/gogoproto) from 1.4.2 to 1.4.3. - [Release notes](https://github.com/cosmos/gogoproto/releases) - [Changelog](https://github.com/cosmos/gogoproto/blob/main/CHANGELOG.md) - [Commits](cosmos/gogoproto@v1.4.2...v1.4.3) --- updated-dependencies: - dependency-name: github.com/cosmos/gogoproto dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Update authz grant examples * Enable larger wasm bytecode upload for gov proposals (CosmWasm#1095) * Enable larger wasm bytecode upload for gov proposals * Set max proposal wasm code size to 3MB * Bump SDK to v0.45.11 * Fix license head * Fix README header * Bump version go to 1.19 (CosmWasm#1044) * bump go 1.19 * add change log * correct change log * Provide source, builder and codehash information in store code proposal message * Implement source, builder, code_info for StoreAndInstantiateProposal * Apply review recommendations * Make linter happy * Fix tests * Formatting only Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Alexander Peters <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Giancarlos Salas <[email protected]> Co-authored-by: pinosu <[email protected]> Co-authored-by: GNaD13 <[email protected]>
Incredibly important work. Thank you! |
…proposal message (CosmWasm#1072) * Provide source, builder and codehash information in store code proposal message * Make linter happy * Update x/wasm/simulation/proposals.go Co-authored-by: Alexander Peters <[email protected]> * Update x/wasm/client/cli/gov_tx.go * Update x/wasm/client/cli/gov_tx.go * Bump github.com/cosmos/gogoproto from 1.4.2 to 1.4.3 Bumps [github.com/cosmos/gogoproto](https://github.com/cosmos/gogoproto) from 1.4.2 to 1.4.3. - [Release notes](https://github.com/cosmos/gogoproto/releases) - [Changelog](https://github.com/cosmos/gogoproto/blob/main/CHANGELOG.md) - [Commits](cosmos/gogoproto@v1.4.2...v1.4.3) --- updated-dependencies: - dependency-name: github.com/cosmos/gogoproto dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Update authz grant examples * Enable larger wasm bytecode upload for gov proposals (CosmWasm#1095) * Enable larger wasm bytecode upload for gov proposals * Set max proposal wasm code size to 3MB * Bump SDK to v0.45.11 * Fix license head * Fix README header * Bump version go to 1.19 (CosmWasm#1044) * bump go 1.19 * add change log * correct change log * Provide source, builder and codehash information in store code proposal message * Implement source, builder, code_info for StoreAndInstantiateProposal * Apply review recommendations * Make linter happy * Fix tests * Formatting only Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Alexander Peters <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Giancarlos Salas <[email protected]> Co-authored-by: pinosu <[email protected]> Co-authored-by: GNaD13 <[email protected]>
…proposal message (CosmWasm#1072) * Provide source, builder and codehash information in store code proposal message * Make linter happy * Update x/wasm/simulation/proposals.go Co-authored-by: Alexander Peters <[email protected]> * Update x/wasm/client/cli/gov_tx.go * Update x/wasm/client/cli/gov_tx.go * Bump github.com/cosmos/gogoproto from 1.4.2 to 1.4.3 Bumps [github.com/cosmos/gogoproto](https://github.com/cosmos/gogoproto) from 1.4.2 to 1.4.3. - [Release notes](https://github.com/cosmos/gogoproto/releases) - [Changelog](https://github.com/cosmos/gogoproto/blob/main/CHANGELOG.md) - [Commits](cosmos/gogoproto@v1.4.2...v1.4.3) --- updated-dependencies: - dependency-name: github.com/cosmos/gogoproto dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Update authz grant examples * Enable larger wasm bytecode upload for gov proposals (CosmWasm#1095) * Enable larger wasm bytecode upload for gov proposals * Set max proposal wasm code size to 3MB * Bump SDK to v0.45.11 * Fix license head * Fix README header * Bump version go to 1.19 (CosmWasm#1044) * bump go 1.19 * add change log * correct change log * Provide source, builder and codehash information in store code proposal message * Implement source, builder, code_info for StoreAndInstantiateProposal * Apply review recommendations * Make linter happy * Fix tests * Formatting only Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Alexander Peters <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Giancarlos Salas <[email protected]> Co-authored-by: pinosu <[email protected]> Co-authored-by: GNaD13 <[email protected]>
This PR's aim is to standardize contract verification on store code proposals.
On store code proposals, proposal creators usually pass code source, builder and code hash information as a part of the description. Let's move these fields back to proposal content and run a check during store to verify provided hash is correct.