Add check that wasm contract only modifies code hash in migrateContract API #5037
Conversation
…wasm-contract-only-modifies-code-hash-in-migratecontract-api
chatton
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
There was a problem hiding this comment.
Nice work, @chatton. I left a few comments after a first pass.
| @@ -242,6 +249,65 @@ func (suite *TypesTestSuite) TestWasmSudo() { | |||
| }, | |||
| types.ErrWasmInvalidResponseData, | |||
| }, | |||
| { | |||
| "failure: invalid clientstate type", | |||
There was a problem hiding this comment.
Should we add similar tests in TestWasmMigrate (except for the one that changes the code hash)?
…wasm-contract-only-modifies-code-hash-in-migratecontract-api
…wasm-contract-only-modifies-code-hash-in-migratecontract-api
…wasm-contract-only-modifies-code-hash-in-migratecontract-api
…wasm-contract-only-modifies-code-hash-in-migratecontract-api
…wasm-contract-only-modifies-code-hash-in-migratecontract-api
There was a problem hiding this comment.
Excellent work! I just left one comment re-raising one suggestion that @DimitrisJim had that I think makes sense. Happy to do it a follow-up PR.
| // codehash should only be able to be modified during migration. | ||
| if !bytes.Equal(codeHash, newClientState.CodeHash) { | ||
| return result, errorsmod.Wrapf(ErrWasmInvalidContractModification, "expected code hash %s, got %s", hex.EncodeToString(codeHash), hex.EncodeToString(newClientState.CodeHash)) | ||
| } |
There was a problem hiding this comment.
@DimitrisJim suggested to do this check also in wasmMigrate and then it could be included in validatePostExecutionClientState, just to make sure the the contract is not doing something we don't expect, even though we set the code hash in 08-wasm after calling wasmMigrate.
There was a problem hiding this comment.
Yea, didn't push this now. I do agree would be nice to do but without real consequence. Open to pushing this afterwards.
|
I am going to merge this now and tag later today beta.0. |
crodriguezvega
deleted the
cian/issue#5011-add-check-that-wasm-contract-only-modifies-code-hash-in-migratecontract-api
branch
Description
closes: #5011
This PR adds checks that verify
Note: a lot of tests needed to be updated as we were using dummy bytes as clientState (which is no longer valid). These tests were updated to use valid
ClientStatebytes.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.