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: add passthrough to genesis #1888

Merged
merged 14 commits into from
Jul 25, 2024
Merged

Conversation

mustermeiszer
Copy link
Collaborator

Description

Currently, we always need to have real Axelar routers on testnets. That prevents us from texting LP logic on development for example.

This PR adds a passthrough router on the EVM side to all new chains. The router always sits at the same location (0x33e7daf228e7613ba85ef6c3647dbceb0f011f7c) and is based on this contract.

The router simply forwards LP messages to the precompile, without checking anything. The AxelarPrecompile of course needs to have 0x33e7daf228e7613ba85ef6c3647dbceb0f011f7c set as its trusted GatewayContract.

Feat

  • Enables testing LP logic on development - making development our main testground and demo will move more to a public testground for external users

Changes and Descriptions

  • Adds runtime_common::evm::utils::account_codes() - generates the genesis codes for the EVM side
  • Adds passthrough router bytecode as constant
  • Adds passthrough router H160 by deriving a hash from a constant input

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

wischli
wischli previously approved these changes Jun 26, 2024
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Great improvement!

@@ -107,3 +107,55 @@ impl<T: pallet_aura::Config<AuthorityId = AuraId>> FindAuthor<H160> for FindAuth
None
}
}

/// Passthrough router deployed bytecode as of this state https://github.com/centrifuge/liquidity-pools/tree/d3da102e7bf656fd50feeb888e17f423317aeeb3
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Which code changes on solidity side lead to a required update of the bytecode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just the ones in the file linked above. Should maybe make that clear in the comment.

lemunozm
lemunozm previously approved these changes Jun 26, 2024
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM! This change does not imply any change/simplification in the current lp tests?

@@ -352,7 +352,7 @@ fn centrifuge_genesis(
"chainId": Into::<u32>::into(chain_id),
},
"evm": {
"accounts": runtime_common::evm::precompile::utils::precompile_account_genesis::<centrifuge_runtime::Precompiles>(),
"accounts": runtime_common::evm::utils::account_genesis::<centrifuge_runtime::Precompiles>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also change this for testing purposes right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wischli we should probably also generate a new dev genesis, right? For the life environment, or don't we use a static one?

@mustermeiszer mustermeiszer dismissed stale reviews from lemunozm and wischli via 1afd37b June 26, 2024 18:40
lemunozm
lemunozm previously approved these changes Jul 24, 2024
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM

b"PASSTHROUGH_ROUTER_ACCOUNT_CODES_ACCOUNT_LOCATION_SALT";

pub fn passthrough_router_location() -> H160 {
H160::from(sp_core::H256::from(sp_core::KeccakHasher::hash(
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, who demands the usage of KeccakHasher?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is just the natural derivation scheme vor evm addresses.

@wischli
Copy link
Contributor

wischli commented Jul 24, 2024

@mustermeiszer Sorry for being intrusive. I can imagine you have tons of stuff on your plate before holiday so I made some minor changes to make CI green.

lemunozm
lemunozm previously approved these changes Jul 24, 2024
Comment on lines 161 to 162
BlakeTwo256::hash_of(&PASSTHROUGH_ROUTER_ACCOUNT_CODES.to_vec()),
hex_literal::hex!("545a48d6f7f1a01cb2bd090a5272cd52c54eafea762071ec652c8ac94610146e")
Copy link
Contributor

Choose a reason for hiding this comment

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

@mustermeiszer This test fails. I suppose due to changes to the PassThroughAdapter.sol between your referenced commit ee35da785f2af3303ef9f52c6ecefeb9671aa61b and current main at 223a0f36edabc675f8c74c47b20e366178df7ca3, see diff.

I wonder how you derived the passthrough account byte codes.

Error

thread 'evm::tests::stable_passthrough_bytecode_hash' panicked at runtime/common/src/evm/mod.rs:160:9:
assertion `left == right` failed
  left: 0x31173f15567854cfc3702aa6b639bf0dedf74638e745a3e90fa00f1619d8b94c
 right: 0x545a48d6f7f1a01cb2bd090a5272cd52c54eafea762071ec652c8ac94610146e

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I can not compile it wanted to retrieve the new hash through the UI. Haha. So using the hash here generated will be okay

Copy link
Contributor

Choose a reason for hiding this comment

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

But how can I derive the PASSTHROUGH_ROUTER_ACCOUNT_CODES?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You just need to swap the hashes

Copy link
Contributor

Choose a reason for hiding this comment

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

But then I only adjust the blake2 256 hash of the PASSTHROUGH_ROUTER_ACCOUNT_CODES but not any of the 3289 bytes of PASSTHROUGH_ROUTER_ACCOUNT_CODES. Based on your comments, those need to be adjusted as well?

/// Passthrough router deployed bytecode as of this state https://github.com/centrifuge/liquidity-pools/blob/ee35da785f2af3303ef9f52c6ecefeb9671aa61b/test/integration/PassthroughAdapter.sol
///
/// NOTE: If the above file changes, this code needs to be adapted.
...

Copy link
Contributor

@wischli wischli Jul 25, 2024

Choose a reason for hiding this comment

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

Does this change logically suffice @mustermeiszer ? 949bdff Technically, it fixes tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that was, what I intended to do! Perfect! We always need that hash for setting up the routers, so I wanted to have it somewhere ^^

@wischli wischli requested a review from hieronx as a code owner July 25, 2024 11:47
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 11.11111% with 16 lines in your changes missing coverage. Please review.

Project coverage is 47.95%. Comparing base (8e714b8) to head (837006f).

Files Patch % Lines
runtime/common/src/evm/mod.rs 13.33% 13 Missing ⚠️
node/src/chain_spec.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1888      +/-   ##
==========================================
- Coverage   48.02%   47.95%   -0.07%     
==========================================
  Files         181      181              
  Lines       13144    13159      +15     
==========================================
- Hits         6313     6311       -2     
- Misses       6831     6848      +17     

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

@wischli wischli merged commit f5ce924 into main Jul 25, 2024
12 checks passed
@wischli wischli deleted the feat/passthrough-router-in-genesis branch July 25, 2024 14:56
wischli added a commit that referenced this pull request Jul 25, 2024
* feat: add passthrough to genesis

* fix: adapt passthrough router

* fix: docs and hash

* chore: rm unused test

* feat: adatpt to latest version

* fix: array size

* fix: clippy + fmt

* fix: update blake hash

* fix: compilation

* fix: std import KeccakHasher

---------

Co-authored-by: William Freudenberger <w.freude@icloud.com>
gpmayorga pushed a commit that referenced this pull request Jul 29, 2024
* feat: add passthrough to genesis

* fix: adapt passthrough router

* fix: docs and hash

* chore: rm unused test

* feat: adatpt to latest version

* fix: array size

* fix: clippy + fmt

* fix: update blake hash

* fix: compilation

* fix: std import KeccakHasher

---------

Co-authored-by: William Freudenberger <w.freude@icloud.com>
@wischli wischli mentioned this pull request Jul 30, 2024
4 tasks
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.

3 participants