Skip to content
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(upgrader): add generalized atomic upgrade and migration capabilities #77

Merged
merged 21 commits into from
Nov 21, 2024

Conversation

cgorenflo
Copy link
Contributor

@cgorenflo cgorenflo commented Nov 15, 2024

@cgorenflo cgorenflo requested a review from a team as a code owner November 15, 2024 22:56
@cgorenflo cgorenflo marked this pull request as draft November 15, 2024 22:59
contracts/axelar-gateway/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/error.rs Outdated Show resolved Hide resolved
contracts/axelar-gateway/src/contract.rs Outdated Show resolved Hide resolved
contracts/axelar-gateway/src/contract.rs Outdated Show resolved Hide resolved
contracts/axelar-gateway/src/contract.rs Outdated Show resolved Hide resolved
contracts/axelar-gateway/src/contract.rs Outdated Show resolved Hide resolved
contracts/axelar-gateway/src/contract.rs Outdated Show resolved Hide resolved
contracts/axelar-gateway/src/error.rs Outdated Show resolved Hide resolved
contracts/upgrader/Cargo.toml Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
# Conflicts:
#	contracts/axelar-gateway/src/contract.rs
#	contracts/axelar-gateway/src/error.rs
contracts/axelar-gateway/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/Cargo.toml Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 90.27237% with 25 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@527c2fb). Learn more about missing BASE report.
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
contracts/axelar-gateway/src/contract.rs 39.47% 23 Missing ⚠️
contracts/upgrader/src/contract.rs 99.52% 1 Missing ⚠️
contracts/upgrader/src/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #77   +/-   ##
=======================================
  Coverage        ?   93.58%           
=======================================
  Files           ?       34           
  Lines           ?     1824           
  Branches        ?        0           
=======================================
  Hits            ?     1707           
  Misses          ?      117           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@cgorenflo cgorenflo marked this pull request as ready for review November 21, 2024 17:45
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
contracts/upgrader/src/contract.rs Outdated Show resolved Hide resolved
@cgorenflo cgorenflo merged commit 48507e2 into main Nov 21, 2024
5 checks passed
@cgorenflo cgorenflo deleted the upgradability branch November 21, 2024 21:03
contracts/axelar-gateway/src/storage_types.rs Show resolved Hide resolved
contracts/upgrader/src/contract.rs Show resolved Hide resolved
contracts/axelar-gateway/src/error.rs Show resolved Hide resolved
contracts/axelar-gateway/src/contract.rs Show resolved Hide resolved
contracts/axelar-gateway/src/contract.rs Show resolved Hide resolved
contracts/upgrader/src/contract.rs Show resolved Hide resolved
let contract_address = env.register(DummyContract, ());
let upgrader_address = env.register(Upgrader, ());

let _owner = set_owner(&env, &contract_address);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a setup_env helper that sets up the env and contract clients, similar to other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need the owner, and contract addresses throughout the code. I find

let owner = Address::generate(&env);
let contract_address = env.register(DummyContract, (&owner,));
let upgrader_address = env.register(Upgrader, ());

cleaner than

let (owner, contract_address, upgrader_address) = setup(&env)

...

fn setup(env: &Env){
   let owner = Address::generate(env);
   let contract_address = env.register(DummyContract, (&owner,));
   let upgrader_address = env.register(Upgrader, ());
}

contracts/upgrader/src/contract.rs Show resolved Hide resolved
},
MockAuth {
address: &owner,
invoke: &migrate_auth,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try using assert_invoke_auth_err instead? You can add another pattern for it where it takes a list of callers and invocation calls and creates mock auths for them, and then calls the final method to test the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the auth that I need to mock here is very specific for this scenario: I call function A, but need to authenticate functions B and C that have very specific subsets of the input parameters. Also, the tests are modulating what is authenticated and what is not, so there is no good macro solution, and it wouldn't be reusable anyway

contracts/upgrader/src/contract.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants