✨[API-2348] Make Contracts Upgradeable #81
Conversation
WalkthroughThe changes across the contracts involve introducing upgradeability to the CosmWasm contracts. This is achieved by adding a Changes
Assessment against linked issues
PoemIn the blockchain's burrow, deep and wide, Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
NotJeremyLiu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files ignored due to filter (12)
- Cargo.lock
- Cargo.toml
- contracts/adapters/ibc/ibc-hooks/Cargo.toml
- contracts/adapters/ibc/neutron-transfer/Cargo.toml
- contracts/adapters/swap/astroport/Cargo.toml
- contracts/adapters/swap/lido-satellite/Cargo.toml
- contracts/adapters/swap/osmosis-poolmanager/Cargo.toml
- contracts/entry-point/Cargo.toml
- schema/skip-api-entry-point.json
- scripts/configs/neutron.toml
- scripts/configs/osmosis.toml
- scripts/configs/terra.toml
Files selected for processing (10)
- contracts/adapters/ibc/ibc-hooks/src/contract.rs (1 hunks)
- contracts/adapters/ibc/neutron-transfer/src/contract.rs (1 hunks)
- contracts/adapters/swap/astroport/src/contract.rs (1 hunks)
- contracts/adapters/swap/lido-satellite/src/contract.rs (1 hunks)
- contracts/adapters/swap/osmosis-poolmanager/src/contract.rs (2 hunks)
- contracts/entry-point/src/contract.rs (1 hunks)
- packages/skip/src/entry_point.rs (1 hunks)
- packages/skip/src/ibc.rs (1 hunks)
- packages/skip/src/swap.rs (1 hunks)
- scripts/deploy.py (14 hunks)
Additional comments: 15
contracts/entry-point/src/contract.rs (2)
31-32: Using environment variables to set
CONTRACT_NAMEandCONTRACT_VERSIONis a standard practice and is correctly implemented here.42-42: The invocation of
set_contract_versionwithin theinstantiatefunction is correct and necessary for contract versioning.contracts/adapters/swap/lido-satellite/src/contract.rs (2)
37-38: Using environment variables to set
CONTRACT_NAMEandCONTRACT_VERSIONis a standard practice and is correctly implemented here.48-48: The invocation of
set_contract_versionwithin theinstantiatefunction is correct and necessary for contract versioning.contracts/adapters/ibc/neutron-transfer/src/contract.rs (1)
- 34-47: Setting the contract version using
CONTRACT_NAMEandCONTRACT_VERSIONderived from environment variables is a good practice for maintaining contract versioning. This change is well-implemented.contracts/adapters/ibc/ibc-hooks/src/contract.rs (1)
- 38-51: Setting the contract version using
CONTRACT_NAMEandCONTRACT_VERSIONderived from environment variables is a good practice for maintaining contract versioning. This change is well-implemented.contracts/adapters/swap/osmosis-poolmanager/src/contract.rs (1)
- 39-52: Setting the contract version using
CONTRACT_NAMEandCONTRACT_VERSIONderived from environment variables is a good practice for maintaining contract versioning. This change is well-implemented.scripts/deploy.py (7)
4-18: The removal of imports such as
b64decode,create_cosmwasm_execute_msg, andCoinsuggests refactoring or removal of functionality that used these imports. Ensure that the removal of these imports does not affect any other parts of the script that may rely on them.40-42: Improved error message for missing configuration is a good practice for clarity and debugging.
68-70: The addition of an explicit exception for unspecified network configuration (
mainnetortestnet) enhances the robustness of the script.86-87: The addition of
ADMIN_ADDRESSin the script is crucial for managing contract migrations and aligns with the PR's objective to make contracts upgradeable.295-311: The
instantiate_contractfunction correctly sets theadminfield to theADMIN_ADDRESS, aligning with the upgradability feature.323-350: The
instantiate2_contractfunction has been introduced, which includes asaltparameter for deterministic contract creation. Ensure that the use ofsaltandfix_msgparameters are well-documented and tested, as they are critical for the deterministic creation of contracts.382-388: The
broadcast_txfunction now includes a delay and a GET request to check the transaction status, which is a good practice to ensure the transaction was processed before proceeding.contracts/adapters/swap/astroport/src/contract.rs (1)
- 41-54: The contract's instantiation now includes setting the contract version using
set_contract_version, which is a necessary step for enabling contract migrations.
|
|
||
| #[cfg_attr(not(feature = "library"), entry_point)] | ||
| pub fn migrate(_deps: DepsMut, _env: Env, _msg: MigrateMsg) -> ContractResult<Response> { | ||
| unimplemented!() |
There was a problem hiding this comment.
Does this not block the upgrade by panicking? Don't you want to just return successfully here?
There was a problem hiding this comment.
Migrations work using the migration code of the new contract you are migrating to, not of the deployed contract. So when we instantiate all the contracts currently in the repo, they will all deploy without ever calling their migrate functions. But then if we want to upgrade a contract in the future, we will do so by implementing that contract's migrate function, store that code on chain, and then submit the MigrateMsg
There was a problem hiding this comment.
tldr: we only implement the migrate function when we are doing a migration/upgrade, but can keep them unimplemented on normal deploys.
This PR
Makes the contracts upgradable by:
Notes
Summary by CodeRabbit
New Features
Refactor
Documentation