[multi-chain] Implement a generic chain spec #653

Xanewok · 2024-09-09T17:26:33Z

Closes #635

For now this does not error out on unknown transaction types and interprets it as EIP-155 for the transactions and EIP-568 for the receipts, doing what we do on main. The integration test listed in #635 passes. What's left is not erroring out on missing nonce/mixHash for remote blocks. EDIT: That's now done in d95eb9a.

@Wodann this is up for preliminary review if you feel like it, I tried to clean it up more or less and retain the existing structure found in edr_optimism.

changeset-bot · 2024-09-09T17:26:37Z

⚠️ No Changeset found

Latest commit: 31f123e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codecov · 2024-09-09T17:42:06Z

Codecov Report

Attention: Patch coverage is 20.84894% with 634 lines in your changes missing coverage. Please review.

Please upload report for BASE (feat/multichain@b694d46). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
crates/edr_generic/src/transaction/request.rs	0.00%	197 Missing ⚠️
crates/edr_generic/src/transaction/signed.rs	8.33%	99 Missing ⚠️
crates/edr_generic/src/eip2718.rs	8.00%	69 Missing ⚠️
crates/edr_generic/src/rpc/block.rs	60.92%	52 Missing and 7 partials ⚠️
crates/edr_generic/src/rpc/transaction.rs	30.15%	41 Missing and 3 partials ⚠️
crates/edr_generic/src/rpc/receipt.rs	48.71%	38 Missing and 2 partials ⚠️
crates/edr_generic/src/spec.rs	8.33%	33 Missing ⚠️
crates/edr_generic/src/receipt/execution.rs	0.00%	30 Missing ⚠️
crates/edr_napi/src/spec.rs	0.00%	26 Missing ⚠️
crates/edr_napi/src/generic_chain.rs	0.00%	24 Missing ⚠️
... and 6 more

Additional details and impacted files

@@                Coverage Diff                 @@
##             feat/multichain     #653   +/-   ##
==================================================
  Coverage                   ?   63.24%           
==================================================
  Files                      ?      210           
  Lines                      ?    23618           
  Branches                   ?    23618           
==================================================
  Hits                       ?    14938           
  Misses                     ?     7766           
  Partials                   ?      914

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

Xanewok · 2024-09-10T12:23:20Z

Ah, d95eb9a regressed the unknown_transaction_types. Reverting to draft while I figure this out.

Xanewok · 2024-09-10T12:50:43Z

Ah, d95eb9a regressed the unknown_transaction_types. Reverting to draft while I figure this out.

Simple mistake, I specialized the block to a concrete tx type but lost support for 32-byte hashes, instead. This brings back the original generic approach and hopefully everything works again 🤞

Wodann

Overall, this looks good! Thank you for the changes. The feedback on the chain spec trait design was also very helpful!

A couple of small comments/suggestions.

Once you have integrated this into Hardhat and validated that the Hardhat test suite passes with the generic chain type, this is good to merge - as far as I am concerned.

Wodann · 2024-09-10T14:37:27Z

crates/edr_generic/src/eip2718.rs

+            // TODO: Should we properly decode the transaction type?
+            TypedEnvelope::Unrecognized(_) => transaction::Type::Unrecognized(0xFF),


We might change this behaviour in Hardhat v3, but in main we don't return the source transaction type number. It gets lost in the conversion to a Legacy transaction. I think we should match that internally for now.

In the future, I would be in favour of returning the source transaction type number.

So I'm torn here, as in practice it shouldn't matter but for example you use the library to RLP-decode the typed envelope and saturate to transaction::Type::Legacy here, it would not be roundtrippable per se; maybe that's okay.

Do you want me to return transaction::Type::Legacy with a note or unreachable! or something else here, specifically?

That's correct. Internally, we never actually decoded anything that we didn't encode ourselves. That's why the guarantee holds in real use cases.

So the tricky case here is the ChainSpecT::PooledTransaction::decode in pub fn handle_send_raw_transaction_request. This should return an invalid transaction type error for any non-supported transaction types. As long as you return that for TypedEnvelope::Unrecognized, we should not be changing behaviour inadvertently.

It seems that the current implementation does that.

Hm, that is tricky - the ChainSpecT::PooledTransaction uses its decode impl but it's only compared against a alloy_rlp::Error::Custom(&'static str) and checks the string contents using a edr_eth::transaction::Signed::is_invalid_transaction_type_error.

While the ProviderSpec impl for GenericChainSpec has the PooledTransaction the same as edr_eth's, so we're safe, the interaction seems somewhat flimsy as we're crossing from the ChainSpec abstraction to the concrete edr_eth impls and use string checks, no less.

I'm also wary of introducing yet another type param but maybe, since we're already using generic and type-safe impls, we should introduce something like TransactionDecodeError and add our InvalidType variant on top of the RLP error or something like that? We seem to be recovering/handling this exact case often enough to uplift it to the type system but I'm curious to see what you think. I can also see a perspective where this is only a single additional variant and it's more trouble than it's worth to pile abstraction over a lean RLP decode trait.

I took the liberty of matching on the const str value rather than using the if guard on the pattern in 31f123e for consistency - I hope you don't mind.

crates/edr_generic/src/lib.rs

crates/edr_generic/src/rpc/block.rs

Wodann · 2024-09-10T14:45:38Z

crates/edr_generic/src/rpc/block.rs

+    use crate::{rpc::transaction::TransactionWithSignature, GenericChainSpec};
+
+    #[tokio::test(flavor = "current_thread")]
+    async fn test_allow_missing_nonce_or_mix_hash() {


Wodann · 2024-09-10T14:52:56Z

crates/edr_generic/src/rpc/transaction.rs

+
+use crate::GenericChainSpec;
+
+// We need to use a newtype here as `RpcTypeFrom` cannot be implemented here,


Good catch!

I'll consider changing the traits in a follow-up PR to avoid this pattern.

See #656

In general there were few cases where it was not possible to re-use the original traits as we did depend on the actual trait implementation or the associated type was set for a given impl, rather than be generic (and so pluggable by potential other impls, like (Fake)Sign).

How often that's the case I'm not sure, as this was a very specific case of "do everything that L1 does except these few tiny bits" but I'd assume that we should provide sensible defaults or building blocks that the other implementers can more easily re-use what we have rather than copy the impls in some cases.

What's the ergonomic solution I'm not yet sure and I'm mostly just thinking out loud. Maybe allow for more generic impls and use the final marker trait impls that tie all of these together to validate that the intermediate types are compatible and expected?

For instance, I found it hard to wrap my head around what to implement as often the traits felt like they were circular, like in the case of satisfying both the RpcSpec and ChainSpec for a type. Another case is that some types like EthBlockData only use a single associated type from the ChainSpec and the ChainSpec feels like this tie-all/validation trait (which makes sense) but it made me harder to understand what's actually used and why. I had trouble deciphering some of the compiler errors and figuring out which trait impls are required because it felt so intertwined.

Maybe I just missed a proper mental model but I have to admit that it took me longer than it should 🙈

crates/edr_generic/src/transaction.rs

Co-authored-by: Wodann <Wodann@users.noreply.github.com>

Xanewok · 2024-09-12T16:49:16Z

I have implemented the ProviderSpec and SyncNapiSpec and re-use the GenericChainSpec instead of the L1ChainSpec in edr_napi and updated the Hardhat patch to use it - let's see what CI says.

Xanewok · 2024-09-12T18:41:32Z

@Wodann I hope everything looks good now! The tests are passing and I applied the patch; let me know if I should add or change anything. Thanks!

Wodann · 2024-09-12T22:15:07Z

While going through the changes I noticed that there was a failing test in the EDR TS bindings but it wasn't being triggered in the CI.

I added CI support, causing the failure, and pushed a fix for it.

I also re-added the implementation of the NapiSpec for L1ChainSpec as that should keep existing for future releases.

Wodann · 2024-09-12T22:16:29Z

crates/edr_napi/src/spec.rs

+    ) -> napi::Result<Response> {
+        let response = jsonrpc::ResponseData::from(response.map(|response| response.result));
+
+        serde_json::to_string(&response)


This code duplication will be removed in https://github.com/NomicFoundation/edr/pull/659/files#diff-bd82f1c79ac0cc9d088b3262a78277433ed1667e6f519c2ac84215247a6fcf61R154

Wodann

I made some changes to fix a failing test that wasn't running in CI. Other than that it LGTM!

There is one open discussion thread regarding a TODO. If my understanding is correct, feel free to proceed with merging, if you're happy with the changes that I made.

This is more consistent with the other uses and increases visibility to the underlying static str value.

Xanewok · 2024-09-13T10:41:05Z

@Wodann thanks for the fixes! For the one last thread I can confirm that it's working as you'd expect but left a note there as well but we can tackle this in a follow-up, if at all. Since the change in 31f123e is cosmetic, I'll go ahead and merge this to unblock your remaining work.

Xanewok temporarily deployed to github-action-benchmark September 9, 2024 17:26 — with GitHub Actions Inactive

Xanewok force-pushed the feat/multichain-generic-chain branch from aef02e2 to e271b2f Compare September 10, 2024 08:51

Xanewok temporarily deployed to github-action-benchmark September 10, 2024 08:51 — with GitHub Actions Inactive

Xanewok added 2 commits September 10, 2024 10:58

feat(wip): Introduce a generic chain spec in the edr_generic crate

2b5af60

feat: Allow for missing nonce/mixHash in remote blocks

d95eb9a

Xanewok force-pushed the feat/multichain-generic-chain branch from e271b2f to d95eb9a Compare September 10, 2024 11:14

Xanewok had a problem deploying to github-action-benchmark September 10, 2024 11:14 — with GitHub Actions Error

Xanewok marked this pull request as ready for review September 10, 2024 12:19

fixup: Formatting

6f57e23

Xanewok temporarily deployed to github-action-benchmark September 10, 2024 12:20 — with GitHub Actions Inactive

Xanewok marked this pull request as draft September 10, 2024 12:23

Xanewok added 2 commits September 10, 2024 14:38

fix: Make sure to accept rpc blocks with txs being 32-byte hashes

192398f

fixup: Maintain symmetry between mod::ConversionErrors used internally

f000874

Xanewok temporarily deployed to github-action-benchmark September 10, 2024 12:49 — with GitHub Actions Inactive

Xanewok marked this pull request as ready for review September 10, 2024 12:50

Wodann mentioned this pull request Sep 10, 2024

Consider updating chain specification traits based on findings from GenericChainSpec implementation #656

Open

Wodann reviewed Sep 10, 2024

View reviewed changes

Update crates/edr_generic/src/lib.rs

75489fb

Co-authored-by: Wodann <Wodann@users.noreply.github.com>

Xanewok had a problem deploying to github-action-benchmark September 10, 2024 19:13 — with GitHub Actions Error

Update crates/edr_generic/src/rpc/block.rs

195b348

Co-authored-by: Wodann <Wodann@users.noreply.github.com>

Xanewok had a problem deploying to github-action-benchmark September 10, 2024 19:13 — with GitHub Actions Error

fixup: Rename SignedWithFallbackToPostEip155

80be89e

Xanewok temporarily deployed to github-action-benchmark September 10, 2024 19:34 — with GitHub Actions Inactive

feat: Implement ProviderSpec for GenericChainSpec

a013fb6

Xanewok temporarily deployed to github-action-benchmark September 12, 2024 13:22 — with GitHub Actions Inactive

feat: Implement SyncNapiSpec for GenericChainSpec and use in edr_napi

fadb7ea

fixup: Re-generate patch hash

59125fb

Xanewok had a problem deploying to github-action-benchmark September 12, 2024 16:44 — with GitHub Actions Error

fixup: Update test/provider.ts

dcecbd1

Xanewok temporarily deployed to github-action-benchmark September 12, 2024 16:58 — with GitHub Actions Inactive

Xanewok requested a review from Wodann September 12, 2024 18:41

Wodann had a problem deploying to github-action-benchmark September 12, 2024 21:26 — with GitHub Actions Error

ci: run EDR TS tests

78f08c6

Wodann force-pushed the feat/multichain-generic-chain branch from a784ee4 to 78f08c6 Compare September 12, 2024 21:41

Wodann had a problem deploying to github-action-benchmark September 12, 2024 21:42 — with GitHub Actions Error

ci: add Alchemy URL

68a59c8

Wodann had a problem deploying to github-action-benchmark September 12, 2024 21:52 — with GitHub Actions Error

Wodann had a problem deploying to github-action-benchmark September 12, 2024 22:07 — with GitHub Actions Error

revert: deletion of L1 chain in N-API

a7cfee7

Wodann force-pushed the feat/multichain-generic-chain branch from 52396d2 to a7cfee7 Compare September 12, 2024 22:11

Wodann temporarily deployed to github-action-benchmark September 12, 2024 22:11 — with GitHub Actions Inactive

Wodann reviewed Sep 12, 2024

View reviewed changes

Wodann self-requested a review September 12, 2024 22:50

Wodann approved these changes Sep 12, 2024

View reviewed changes

ci: cache RPC calls in TS tests

ad3e449

Wodann temporarily deployed to github-action-benchmark September 12, 2024 22:58 — with GitHub Actions Inactive

refactor: Prefer const in pattern match over a guard in tx RLP decode

31f123e

This is more consistent with the other uses and increases visibility to the underlying static str value.

Xanewok temporarily deployed to github-action-benchmark September 13, 2024 10:25 — with GitHub Actions Inactive

Xanewok merged commit 05a8cb6 into feat/multichain Sep 13, 2024
40 of 41 checks passed

Xanewok deleted the feat/multichain-generic-chain branch September 13, 2024 10:42

Xanewok mentioned this pull request Sep 13, 2024

Add generic chain type #635

Closed

github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multi-chain] Implement a generic chain spec #653

[multi-chain] Implement a generic chain spec #653

Xanewok commented Sep 9, 2024 •

edited

Loading

changeset-bot bot commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

Xanewok commented Sep 10, 2024

Xanewok commented Sep 10, 2024

Wodann left a comment

Wodann Sep 10, 2024

Xanewok Sep 12, 2024

Wodann Sep 12, 2024 •

edited

Loading

Xanewok Sep 13, 2024 •

edited

Loading

Wodann Sep 10, 2024

Wodann Sep 10, 2024

Xanewok Sep 12, 2024

Xanewok commented Sep 12, 2024

Xanewok commented Sep 12, 2024

Wodann commented Sep 12, 2024

Wodann Sep 12, 2024 •

edited

Loading

Wodann left a comment

Xanewok commented Sep 13, 2024

		// TODO: Should we properly decode the transaction type?
		TypedEnvelope::Unrecognized(_) => transaction::Type::Unrecognized(0xFF),


		use crate::GenericChainSpec;

		// We need to use a newtype here as `RpcTypeFrom` cannot be implemented here,

[multi-chain] Implement a generic chain spec #653

[multi-chain] Implement a generic chain spec #653

Conversation

Xanewok commented Sep 9, 2024 • edited Loading

changeset-bot bot commented Sep 9, 2024 • edited Loading

⚠️ No Changeset found

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

Xanewok commented Sep 10, 2024

Xanewok commented Sep 10, 2024

Wodann left a comment

Choose a reason for hiding this comment

Wodann Sep 10, 2024

Choose a reason for hiding this comment

Xanewok Sep 12, 2024

Choose a reason for hiding this comment

Wodann Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Xanewok Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Wodann Sep 10, 2024

Choose a reason for hiding this comment

Wodann Sep 10, 2024

Choose a reason for hiding this comment

Xanewok Sep 12, 2024

Choose a reason for hiding this comment

Xanewok commented Sep 12, 2024

Xanewok commented Sep 12, 2024

Wodann commented Sep 12, 2024

Wodann Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Wodann left a comment

Choose a reason for hiding this comment

Xanewok commented Sep 13, 2024

Xanewok commented Sep 9, 2024 •

edited

Loading

changeset-bot bot commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

Wodann Sep 12, 2024 •

edited

Loading

Xanewok Sep 13, 2024 •

edited

Loading

Wodann Sep 12, 2024 •

edited

Loading