feat: add ethereum to cosmos relayer module #190

gjermundgaraba · 2024-12-30T23:26:45Z

Description

closes: #152

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
Wrote unit and integration tests.
Added relevant natspec and godoc comments.
Provide a conventional commit message to follow the repository standards.
Re-reviewed Files changed in the GitHub PR explorer.
Review SonarCloud Report in the comment section below once CI passes.

…module

codecov · 2024-12-30T23:50:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.97%. Comparing base (b77c838) to head (747464b).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #190   +/-   ##
=======================================
  Coverage   96.97%   96.97%           
=======================================
  Files          11       11           
  Lines         562      562           
=======================================
  Hits          545      545           
  Misses         17       17

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

gjermundgaraba · 2025-01-01T12:18:39Z

.gitignore

@@ -27,6 +27,8 @@ broadcast/*/31337/
 target
 elf
 artifacts
+bacon.toml


A bit unrelated, but added these since I am experimenting with running bacon-ls as a wrapper for rust-analyzer to improve incremental compilation.

gjermundgaraba · 2025-01-01T12:20:22Z

Cargo.toml

@@ -82,13 +86,17 @@ ibc-core-commitment-types   = { version = "0.56", default-features = false }
 ibc-core-handler-types      = { version = "0.56", default-features = false }
 ibc-client-tendermint-types = { version = "0.56", default-features = false }

-alloy                  = { version = "0.8", default-features = false }


We can discuss this, but I just felt it was a bit much to depend on the whole thing, when we can just pick and choose the bits we need.

This was not being used on contracts or on chain logic. So, here, readability is more important than picking an choosing. Many of the example codes from alloy also doesn't pick and choose.

So I'd prefer if we only picked and chose in wasm contracts or sp1-programs.

gjermundgaraba · 2025-01-01T12:23:26Z

e2e/interchaintestv8/e2esuite/suite.go

+
+// TODO: Remove when we have removed manual relaying and moved to
+// common relaying methods that can choose the correct methods
+func (s *TestSuite) SkipIfEthTestnetType(testnetType string) {


Not all the e2e relaying tests work with both pow and pos, so added this for now.
I think it might make sense to get rid of the manual relaying in go and switch to the relayer now, but we can still have the relayer calls behind some functions that can handle the different testnet types - and then we can remove this method.

If we agree on this, I'll create an issue to remove the manual relaying and consolidate relaying in the e2e tests.

I think you should add a mock option in the relayer config so that we don't need to do this.

I still think consolidation the tests would be useful, because I don't want to maintain the manual relayer and a bunch of extra tests if we don't have to. If we can just test everything together it would mean quite a bit less code.

But I do like the mock option a lot! :)

packages/ethereum/tree_hash/src/impls.rs

srdtrk

Looks very good. I think you should be very familiar with the relayer now. I have a couple suggestions and questions, so I will approve once those are resolved. And I have a few requests as well:

Can you update config.example.json with the new module.
Can you update the README.md for the relayer.

srdtrk · 2025-01-03T05:33:43Z

Cargo.toml

@@ -82,13 +86,17 @@ ibc-core-commitment-types   = { version = "0.56", default-features = false }
 ibc-core-handler-types      = { version = "0.56", default-features = false }
 ibc-client-tendermint-types = { version = "0.56", default-features = false }

-alloy                  = { version = "0.8", default-features = false }


This was not being used on contracts or on chain logic. So, here, readability is more important than picking an choosing. Many of the example codes from alloy also doesn't pick and choose.

So I'd prefer if we only picked and chose in wasm contracts or sp1-programs.

srdtrk · 2025-01-03T05:35:00Z

e2e/interchaintestv8/e2esuite/suite.go

+
+// TODO: Remove when we have removed manual relaying and moved to
+// common relaying methods that can choose the correct methods
+func (s *TestSuite) SkipIfEthTestnetType(testnetType string) {


I think you should add a mock option in the relayer config so that we don't need to do this.

srdtrk · 2025-01-03T05:36:20Z

e2e/interchaintestv8/relayer/config.go

 	"os"
 	"text/template"
 )

-// EthToCosmosConfigInfo is a struct that holds the configuration information for the Ethereum to Cosmos config template
-type EthToCosmosConfigInfo struct {
+type EthCosmosConfigInfo struct {


In order to support mock wasm contract, perhaps we can add a new field?

srdtrk · 2025-01-03T05:38:27Z

e2e/interchaintestv8/relayer_test.go

+		beaconAPI := ""
+		if eth.BeaconAPIClient != nil {
+			beaconAPI = eth.BeaconAPIClient.GetBeaconAPIURL()
+		}


This is here because of the pow vs. pos tests right?

That is correct. I'll add comment.

srdtrk · 2025-01-03T05:42:53Z

e2e/interchaintestv8/relayer_test.go

@@ -299,6 +331,7 @@ func (s *RelayerTestSuite) Test_5_BatchedAckPacketToEth_Plonk() {
 func (s *RelayerTestSuite) ICS20TransferERC20TokenBatchedAckTest(
 	ctx context.Context, proofType operator.SupportedProofType, numOfTransfers int,
 ) {
+	s.SkipIfEthTestnetType(testvalues.EthTestnetTypePoS)


Can we create an issue to remove these? I think we can do this my adding a hidden mock option to the relayer module?

It would be good to do this in this PR but I understand if you want to create a separate issue for this

Yeah, I did want to get rid of them, I'll create a placeholder issue for it and with needs-discussion until we decide on the approach. A mock option seems like a good idea.

Create an issue. I also realized we don't actually have to consolidate the e2e tests at the same time as we fix this, so that issue should be ready to go: #192

srdtrk · 2025-01-03T06:30:21Z

packages/relayer-lib/src/tx_builder/eth_to_cosmos.rs

+        let mut headers = vec![];
+        let mut trusted_slot = ethereum_consensus_state.slot;
+        let mut prev_pub_agg_key = BlsPublicKey::default();
+        for update in &light_client_updates {


I wonder if this for loop can be done with futures fold instead but this looks good for now

srdtrk · 2025-01-03T06:35:02Z

packages/sp1-ics07-tendermint-utils/Cargo.toml

-alloy                            = { workspace = true, features = ["full", "node-bindings"] }
+alloy-network                    = { workspace = true }
+alloy-signer-local               = { workspace = true }


This is fine since this is a util library

srdtrk · 2025-01-03T06:36:18Z

programs/relayer/Cargo.toml

-alloy = { workspace = true, features = ["full", "node-bindings"] }
+alloy-primitives = { workspace = true }
+alloy-provider   = { workspace = true, features = ["reqwest"] }
+alloy-network    = { workspace = true }
+alloy-transport  = { workspace = true }


This is not needed since this compiles to an executable anyway (no on chain logic).

srdtrk · 2025-01-03T06:37:42Z

programs/relayer/src/modules/cosmos_to_eth.rs

+        tracing::info!("Got {} source_tx_ids", inner_req.source_tx_ids.len());
+        tracing::info!("Got {} timeout_tx_ids", inner_req.timeout_tx_ids.len());


does this format match our logs in other relayer modules? It would be nice to use the same format

More or less, but the other ones didn't have this logging, so changing them a little and adding it to those as well for consistency.

srdtrk · 2025-01-03T06:40:10Z

programs/relayer/src/modules/eth_to_cosmos.rs

+/// The configuration for the Cosmos to Cosmos relayer module.
+#[derive(Clone, Debug, serde::Deserialize, serde::Serialize)]
+#[allow(clippy::module_name_repetitions)]
+pub struct EthToCosmosConfig {


We can add a mock field here (which we allow to be absent in the actual config file and defaults to false)

Co-authored-by: srdtrk <[email protected]>

gjermundgaraba added 14 commits December 22, 2024 23:26

add eth to cosmos module + refactor out tendermint proofs

68ca968

chain info e2e test

841dda7

impl: initial eth -> cosmos relaying

ddb8258

fix chain info test

b832542

eth to cosmos e2e test working

3c4e9c9

lint

85f9b01

remove git hash version from relayer verison

15a7f18

Merge branch 'main' into gjermund/152-add-ethereum-to-cosmos-relayer-…

2003d5f

…module

Add new e2e test to e2e workflow

a9e9e3c

Fix relayer config, use correct relayer client + skip on testnet types

c83ab1f

fix kurtosis version + add e2e test

b54cee0

clean up, linting, etc

5bed95a

wait for condition

8bee100

fix e2e test + add wait_for_condition util

132c38b

gjermundgaraba linked an issue Dec 30, 2024 that may be closed by this pull request

Add Ethereum to Cosmos relayer module #152

Open

cleanup

34b1908

gjermundgaraba marked this pull request as ready for review December 31, 2024 00:23

gjermundgaraba requested a review from srdtrk as a code owner December 31, 2024 00:23

gjermundgaraba commented Jan 1, 2025

View reviewed changes

srdtrk reviewed Jan 3, 2025

View reviewed changes

gjermundgaraba mentioned this pull request Jan 3, 2025

Get rid of skipping on eth testnet type #192

Open

gjermundgaraba and others added 2 commits January 3, 2025 12:22

Update e2e/interchaintestv8/relayer_test.go

8e46750

Co-authored-by: srdtrk <[email protected]>

some minor cleanup

747464b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ethereum to cosmos relayer module #190

feat: add ethereum to cosmos relayer module #190

gjermundgaraba commented Dec 30, 2024

codecov bot commented Dec 30, 2024 •

edited

Loading

gjermundgaraba Jan 1, 2025

gjermundgaraba Jan 1, 2025

srdtrk Jan 3, 2025

gjermundgaraba Jan 1, 2025

srdtrk Jan 3, 2025

gjermundgaraba Jan 3, 2025

gjermundgaraba Jan 3, 2025

gjermundgaraba Jan 3, 2025

srdtrk left a comment

srdtrk Jan 3, 2025

srdtrk Jan 3, 2025

srdtrk Jan 3, 2025

srdtrk Jan 3, 2025

gjermundgaraba Jan 3, 2025

srdtrk Jan 3, 2025

gjermundgaraba Jan 3, 2025

gjermundgaraba Jan 3, 2025

srdtrk Jan 3, 2025

srdtrk Jan 3, 2025

srdtrk Jan 3, 2025

srdtrk Jan 3, 2025

gjermundgaraba Jan 3, 2025

srdtrk Jan 3, 2025

		tracing::info!("Got {} source_tx_ids", inner_req.source_tx_ids.len());
		tracing::info!("Got {} timeout_tx_ids", inner_req.timeout_tx_ids.len());

feat: add ethereum to cosmos relayer module #190

Are you sure you want to change the base?

feat: add ethereum to cosmos relayer module #190

Conversation

gjermundgaraba commented Dec 30, 2024

Description

codecov bot commented Dec 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdtrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 30, 2024 •

edited

Loading