Integrate contract migration #160

dzmitryhil · 2024-02-20T12:59:26Z

Description

Integrate contract migration

Migrate the contract before the integration tests start
Add migration integration test
Add migration CLI and instructions

Reviewers checklist:

Try to write more meaningful comments with clear actions to be taken.
Nit-picking should be unblocking. Focus on core issues.

Authors checklist

Provide a concise and meaningful description
Review the code yourself first, before making the PR.
Annotate your PR in places that require explanation.
Think and try to split the PR to smaller PR if it is big.

This change is

* Migrate contract before the integration tests start * Add migration integration test * Add migration CLI and instructions

miladz68

Reviewed 23 of 25 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

ysv

Reviewed 23 of 25 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @dzmitryhil and @wojtek-coreum)

Makefile line 18 at r2 (raw file):

GOARCH?=
BINARY_NAME?=coreumbridge-xrpl-relayer
MAINNET_RELEASE_VERSION=v0.0.2

nit: do we necessary need it to be released to mainnet ?
I think better name here is smth like upgrade_from_version ?

Code quote:

MAINNET_RELEASE_VERSION

README.md line 227 at r2 (raw file):

#### Download new version of the contract

Download new version form the [Releases page](https://github.com/CoreumFoundation/coreumbridge-xrpl/releases) and store

nit: from the

Code quote:

form the

README.md line 251 at r2 (raw file):

Only owner(admin) is authorized to call that command.

I'm a bit confused
owner & admin are 2 different roles responsible for different things, aren't they ?

Code quote:

Only owner(admin)

.github/workflows/contract-ci.yml line 4 at r2 (raw file):

on:
  push:
    branches: [ master, migration ]

nit: will this be removed before master merge ?

Code quote:

migration

contract/src/migration.rs line 16 at r2 (raw file):

#[entry_point]
pub fn migrate(deps: DepsMut, _env: Env, _msg: MigrateMsg) -> Result<Response, ContractError> {

by the way, is it possible to test migrations on rust side ? how is it usually done @keyleu ?

Sounds like reasonable task for rust because it should be possible to reproduce more complex and specific state on rust end and then try to run migration

I'm ok to keep go testing but in general it seems to be reasonable task for contract creator also.

integration-tests/contract.go line 18 at r2 (raw file):

// CompiledContractFilePath is bridge contract file path.
const (
	MainnetContractFilePath  = "../../build/coreumbridge_xrpl_v0.0.2.wasm"

nit: as I mentioned earlier I don't think mainnet is the best word here. Because mainnet is network but contract version is independent on it

Or we can just use version inside name like we do for cored in crust
something like: ContractFilePathV002

Code quote:

Mainnet

integration-tests/contract.go line 24 at r2 (raw file):

// DeployInstantiateAndMigrateContract deploys and instantiates the mainnet version of the contract and applies
// migration.
func DeployInstantiateAndMigrateContract(

nit: it is kinda bad practice when single func has multiple verbs. It was negligible when we had 2 but 3 is too much.

Maybe we should rename it ? My options: SetupContract or PrepareContract

integration-tests/contract.go line 65 at r2 (raw file):

// DeployAndInstantiateMainnetContract deploys and instantiates the mainnet version of the contract.
func DeployAndInstantiateMainnetContract(

I think ContractFilePath could become parameter and then we can use this func for any version of contract

Or config could also change ?

integration-tests/contract/cancel_pending_operation_test.go line 37 at r2 (raw file):

	})

	owner, contractClient := integrationtests.DeployInstantiateAndMigrateContract(

question: so now in each test instead of just instantiating latest contract version you setup old version then migrate to latest and only after this run integration tests ?

Code quote:

DeployInstantiateAndMigrateContract

relayer/cmd/cli/cli.go line 207 at r2 (raw file):

		contractByteCodePath string,
	) (*sdk.TxResponse, uint64, error)
	MigrateContract(

it is usually named migrate, not upgrade ?

Code quote:

MigrateContract

relayer/cmd/cli/cli.go line 1858 at r2 (raw file):

func MigrateContractCmd(bcp BridgeClientProvider) *cobra.Command {
	cmd := &cobra.Command{
		Use:   "migrate-contract [code-id]",

could you remind me what we decided to do if we migrate to new contract version but relayers still run old one ? Do we block them or allow to submit msgs normally ?

dzmitryhil

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @keyleu and @wojtek-coreum)

contract/src/migration.rs line 16 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

by the way, is it possible to test migrations on rust side ? how is it usually done @keyleu ?

Sounds like reasonable task for rust because it should be possible to reproduce more complex and specific state on rust end and then try to run migration

I'm ok to keep go testing but in general it seems to be reasonable task for contract creator also.

Yes, it is possible with the test-tube. It works similarly to what we do with the znet.

keyleu

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

contract/src/migration.rs line 16 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

by the way, is it possible to test migrations on rust side ? how is it usually done @keyleu ?

Sounds like reasonable task for rust because it should be possible to reproduce more complex and specific state on rust end and then try to run migration

I'm ok to keep go testing but in general it seems to be reasonable task for contract creator also.

Yes, we can test migrations either using multi-test or test-tube like we are currently testing. But it's only done if there is actual migration of the state, if there is no migration logic there is nothing to test.

dzmitryhil

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @wojtek-coreum and @ysv)

Makefile line 18 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: do we necessary need it to be released to mainnet ?
I think better name here is smth like upgrade_from_version ?

Update to RELEASE_VERSION

README.md line 227 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: from the

Done

README.md line 251 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I'm a bit confused
owner & admin are 2 different roles responsible for different things, aren't they ?

Owner - contract manager (logic)
Admin - who is able to redeploy

.github/workflows/contract-ci.yml line 4 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: will this be removed before master merge ?

Yes

integration-tests/contract.go line 18 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: as I mentioned earlier I don't think mainnet is the best word here. Because mainnet is network but contract version is independent on it

Or we can just use version inside name like we do for cored in crust
something like: ContractFilePathV002

Agree, updated.

integration-tests/contract.go line 24 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: it is kinda bad practice when single func has multiple verbs. It was negligible when we had 2 but 3 is too much.

Maybe we should rename it ? My options: SetupContract or PrepareContract

We have 3 different functions which we use. With different actions, I agree, that it would be better to have better names, but I don't know better.

integration-tests/contract.go line 65 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I think ContractFilePath could become parameter and then we can use this func for any version of contract

Or config could also change ?

The difference is that the deployment might be different (not it is the same just for an example, but later the init config will be different is it will be there at all), and passing the same contract path in all tests is redundant.

integration-tests/contract/cancel_pending_operation_test.go line 37 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

question: so now in each test instead of just instantiating latest contract version you setup old version then migrate to latest and only after this run integration tests ?

Correct, the intention is to check the backward compatibility.

relayer/cmd/cli/cli.go line 207 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

it is usually named migrate, not upgrade ?

Migrate

relayer/cmd/cli/cli.go line 1858 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

could you remind me what we decided to do if we migrate to new contract version but relayers still run old one ? Do we block them or allow to submit msgs normally ?

We plan to keep it backward-compatible for now. Or they will update the relayer docker image which will be able to work with both old and new.

ysv

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil and @wojtek-coreum)

README.md line 251 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Owner - contract manager (logic)
Admin - who is able to redeploy

yes, but here you wrote:

Only owner(admin) is authorized to call that command.

So from this line it is not clear who is authorized.

contract/src/migration.rs line 16 at r2 (raw file):

if there is actual migration of the state

Yes exactly, That is what I mean. So once we have state migration lets not forget to add tests for it.
For now since it is still non-released version it is ok

integration-tests/contract.go line 65 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

The difference is that the deployment might be different (not it is the same just for an example, but later the init config will be different is it will be there at all), and passing the same contract path in all tests is redundant.

then it should be smth like DeployAndInstantiateContrantV002
Again lets avoid Mainnet everywhere

dzmitryhil

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wojtek-coreum and @ysv)

README.md line 251 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

yes, but here you wrote:

Only owner(admin) is authorized to call that command.

So from this line it is not clear who is authorized.

Agree, updated. Only admin is authorized to call that command.

integration-tests/contract.go line 65 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

then it should be smth like DeployAndInstantiateContrantV002
Again lets avoid Mainnet everywhere

Done

ysv

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

miladz68

Reviewed 2 of 4 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum)

# Conflicts: # README.md # relayer/client/bridge.go # relayer/cmd/cli/cli.go # relayer/cmd/main.go

The base branch was changed.

miladz68

Reviewed 22 of 22 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @keyleu, @wojtek-coreum, and @ysv)

keyleu

Reviewed 5 of 25 files at r1, 22 of 22 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wojtek-coreum and @ysv)

wojtek-coreum

Reviewed 5 of 25 files at r1, 20 of 22 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil and @ysv)

integration-tests/contract/cancel_pending_operation_test.go line 37 at r6 (raw file):

	})

	owner, contractClient := integrationtests.DeployInstantiateAndMigrateContract(

do you need to migrate contract in every integration test?

integration-tests/processes/env_test.go line 163 at r6 (raw file):

		)
		require.NoError(t, err)
		require.NoError(t, contractClient.SetContractAddress(contractAddress))

could you pass contract address using a constructor?

relayer/coreum/contract.go line 586 at r6 (raw file):

	byteCode []byte,
) (*sdk.TxResponse, uint64, error) {
	txRes, codeID, err := c.deployContract(ctx, sender, byteCode)

Can't you simply put the body of c.deployContract here and use the exported method everywhere?

dzmitryhil

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @wojtek-coreum and @ysv)

integration-tests/contract/cancel_pending_operation_test.go line 37 at r6 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

do you need to migrate contract in every integration test?

To be sure that the new contract is backward compatible - yes.

integration-tests/processes/env_test.go line 163 at r6 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

could you pass contract address using a constructor?

We deploy the contract and instantiate using the same client. So we don't have the address at the time we create it.

relayer/coreum/contract.go line 586 at r6 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Can't you simply put the body of c.deployContract here and use the exported method everywhere?

Done.

wojtek-coreum

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ysv)

keyleu

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ysv)

Integrate contract migration

4051b0e

* Migrate contract before the integration tests start * Add migration integration test * Add migration CLI and instructions

dzmitryhil requested a review from a team as a code owner February 20, 2024 12:59

dzmitryhil requested review from miladz68, ysv and wojtek-coreum and removed request for a team February 20, 2024 12:59

dzmitryhil changed the base branch from master to migration February 20, 2024 12:59

dzmitryhil added 2 commits February 20, 2024 17:26

Add curl faild

edfa527

Add matrix.build-contract check for download-released-contract step

ec5b3b5

miladz68 approved these changes Feb 26, 2024

View reviewed changes

ysv requested changes Feb 26, 2024

View reviewed changes

dzmitryhil commented Feb 26, 2024

View reviewed changes

keyleu reviewed Feb 26, 2024

View reviewed changes

dzmitryhil commented Feb 26, 2024

View reviewed changes

Update naming

652193e

ysv requested changes Feb 27, 2024

View reviewed changes

dzmitryhil commented Feb 27, 2024

View reviewed changes

Update naming

469d32d

ysv previously approved these changes Feb 27, 2024

View reviewed changes

miladz68 previously approved these changes Mar 8, 2024

View reviewed changes

dzmitryhil added 2 commits March 14, 2024 09:39

Merge branch 'master' into dzmitryhil/add-contract-migration-support

3a5a62a

# Conflicts: # README.md # relayer/client/bridge.go # relayer/cmd/cli/cli.go # relayer/cmd/main.go

Merge master and address conflicts

8394c68

dzmitryhil changed the base branch from migration to master March 14, 2024 08:01

dzmitryhil added 2 commits March 14, 2024 11:02

Empty-Commit

51f8441

Fix TestDeployContractCmd test

16050bc

dzmitryhil requested review from keyleu, miladz68 and ysv March 14, 2024 08:31

Merge branch 'master' into dzmitryhil/add-contract-migration-support

8aac461

miladz68 previously approved these changes Mar 14, 2024

View reviewed changes

Update method typo

73f2620

dzmitryhil dismissed miladz68’s stale review via 73f2620 March 14, 2024 12:32

keyleu previously approved these changes Mar 14, 2024

View reviewed changes

wojtek-coreum requested changes Mar 15, 2024

View reviewed changes

Remove duplicated function

d86743a

dzmitryhil commented Mar 15, 2024

View reviewed changes

dzmitryhil dismissed keyleu’s stale review via d86743a March 15, 2024 11:34

wojtek-coreum approved these changes Mar 18, 2024

View reviewed changes

Merge branch 'master' into dzmitryhil/add-contract-migration-support

e38a8b8

keyleu approved these changes Mar 18, 2024

View reviewed changes

dzmitryhil merged commit 8e30198 into master Mar 18, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate contract migration #160

Integrate contract migration #160

dzmitryhil commented Feb 20, 2024 •

edited by CoreumDevelopment

Loading

miladz68 left a comment

ysv left a comment

dzmitryhil left a comment

keyleu left a comment

dzmitryhil left a comment

ysv left a comment

dzmitryhil left a comment

ysv left a comment

miladz68 left a comment

miladz68 left a comment

keyleu left a comment

wojtek-coreum left a comment

dzmitryhil left a comment

wojtek-coreum left a comment

keyleu left a comment

Integrate contract migration #160

Integrate contract migration #160

Conversation

dzmitryhil commented Feb 20, 2024 • edited by CoreumDevelopment Loading

Description

Reviewers checklist:

Authors checklist

miladz68 left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

keyleu left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

miladz68 left a comment

Choose a reason for hiding this comment

keyleu left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

keyleu left a comment

Choose a reason for hiding this comment

dzmitryhil commented Feb 20, 2024 •

edited by CoreumDevelopment

Loading