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

ScheduleUpgrade connectors message #1469

Merged
merged 37 commits into from Aug 24, 2023
Merged

Conversation

AStox
Copy link
Contributor

@AStox AStox commented Jul 27, 2023

Description

Adds the ScheduleRely message, intended for governance to rely and execute spells on EVM domain gateway contracts

Changes and Descriptions

  • Adds a ScheduleRely(user: Address) message.
  • Adds a schedule_rely function to lib.rs

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@AStox AStox requested a review from NunoAlexandre July 27, 2023 20:53
Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

One typo that's breaking the build (if I don't miss anything else) and apart from that we will then also need to run cargo fmt and taplo fmt.

Regarding the test, once the build is fixed the test will fail since it will try to compare the resulting hex the message encoded to against ""; we can then use that hex-encoded call and replace the "" with that.

pallets/connectors/src/lib.rs Outdated Show resolved Hide resolved
@hieronx
Copy link
Contributor

hieronx commented Aug 7, 2023

We renamed this to AddAdmin on the Solidity side, so we should do the same here.

@AStox
Copy link
Contributor Author

AStox commented Aug 9, 2023

Actually we mostly (but not fully) renamed this set of features in solidity to "scheduling relies". I've just pushed a PR to rename the rest of the addAdmin mentions in solidity to scheduleRely @offerijns

pallets/connectors/src/lib.rs Outdated Show resolved Hide resolved
) -> DispatchResult {
let who = ensure_signed(origin)?;

T::OutboundQueue::submit(
Copy link
Contributor

Choose a reason for hiding this comment

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

and here we should ensure that the the DomainAddress is a DomainAddress::EVM, i.e, this should fail with an error when it's DomainAddress::Centrifuge since we don't want to be sending this message to the chain itself.

@AStox AStox changed the title Schedule rely connectors message ScheduleUpgrade connectors message Aug 10, 2023
@AStox AStox marked this pull request as ready for review August 10, 2023 16:42
Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

A nit 👀

pallets/liquidity-pools/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

I promise we are almost there 🙏

Comment on lines 700 to 708
// get account id of this pallet
let who = T::PalletId::get().into_account_truncating();
ensure!(
contract.domain() != Domain::Centrifuge,
Error::<T>::InvalidDomain
);

T::OutboundQueue::submit(
who,
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit but just a suggestion:

We don't need who until we call submit and it's only used there, so we could do:

Suggested change
// get account id of this pallet
let who = T::PalletId::get().into_account_truncating();
ensure!(
contract.domain() != Domain::Centrifuge,
Error::<T>::InvalidDomain
);
T::OutboundQueue::submit(
who,
ensure!(
contract.domain() != Domain::Centrifuge,
Error::<T>::InvalidDomain
);
T::OutboundQueue::submit(
T::PalletId::get().into_account_truncating(),

pallets/liquidity-pools/src/lib.rs Outdated Show resolved Hide resolved
mustermeiszer
mustermeiszer previously approved these changes Aug 21, 2023
@mustermeiszer
Copy link
Collaborator

What will be triggered on the Solidity if this message is received?

mustermeiszer
mustermeiszer previously approved these changes Aug 21, 2023
@NunoAlexandre NunoAlexandre enabled auto-merge (squash) August 21, 2023 11:15
@NunoAlexandre
Copy link
Contributor

What will be triggered on the Solidity if this message is received?

@offerijns will explain this better but, afaik, this will schedule a contract upgrade (Solidity "spell") on the EVM side. the contract field of this new ScheduleUpgrade points to that upgrade/spell.

I guess a fair analogy would be to schedule a runtime upgrade and pass on the BLAKE2_256 of the new wasm (if the wasm was living somewhere in the network like a contract does)

@hieronx
Copy link
Contributor

hieronx commented Aug 21, 2023

I guess a fair analogy would be to schedule a runtime upgrade and pass on the BLAKE2_256 of the new wasm (if the wasm was living somewhere in the network like a contract does)

Yes basically this, except the hash being passed is not the new code version itself, but rather a hash of the runtime upgrade that is executing the changes.

mustermeiszer
mustermeiszer previously approved these changes Aug 21, 2023
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Re-approve

@NunoAlexandre
Copy link
Contributor

@offerijns @mustermeiszer can we get another re-approval here? I had forgotten to include 49f68f9 in the last run

mustermeiszer
mustermeiszer previously approved these changes Aug 22, 2023
@NunoAlexandre NunoAlexandre merged commit d651ced into main Aug 24, 2023
11 checks passed
@NunoAlexandre NunoAlexandre deleted the scheduleRely-connectors-message branch August 24, 2023 17:18
@wischli
Copy link
Contributor

wischli commented Aug 25, 2023

It would be great if we did not bump dependencies silently as part of new features. They can introduce silent errors and thus should ideally only be packaged with Substrate updates.

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.

None yet

6 participants