Skip to content

Replace ethabi#2282

Merged
kostko merged 2 commits into
mainfrom
andrej/feature/replace-ethabi
Sep 9, 2025
Merged

Replace ethabi#2282
kostko merged 2 commits into
mainfrom
andrej/feature/replace-ethabi

Conversation

@abukosek
Copy link
Copy Markdown
Contributor

@abukosek abukosek commented Jul 21, 2025

Closes #2274.

Since the ethabi crate is no longer maintained and the new version of evm has a dependency incompatibility with it, we should switch to something else.

The ethabi crate recommends solabi as the replacement, so let's try that...

This PR changes the majority of ethabi uses into solabi, but a few have been left for @njelich to do while I'm on vacation :)
(Those are the evm_derive macros and erc20 precompiles [and tests related to them], as well as a few bits here and there that I probably missed.)

@netlify
Copy link
Copy Markdown

netlify Bot commented Jul 21, 2025

Deploy Preview for oasisprotocol-oasis-sdk canceled.

Name Link
🔨 Latest commit 09d757e
🔍 Latest deploy log https://app.netlify.com/projects/oasisprotocol-oasis-sdk/deploys/68c02ace5e0c340008aa8aa9

@abukosek abukosek force-pushed the andrej/feature/replace-ethabi branch 2 times, most recently from 5d84037 to c3a0267 Compare July 21, 2025 12:46
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 22, 2025

Codecov Report

❌ Patch coverage is 72.56098% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.66%. Comparing base (7c3b1fb) to head (02678ea).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
runtime-sdk/modules/evm/src/types.rs 23.80% 16 Missing ⚠️
runtime-sdk/modules/rofl-market/src/payment.rs 0.00% 15 Missing ⚠️
...ime-sdk/modules/evm/src/precompile/confidential.rs 80.55% 7 Missing ⚠️
runtime-sdk-macros/src/evm_derive.rs 54.54% 5 Missing ⚠️
runtime-sdk/modules/evm/src/precompile/subcall.rs 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2282      +/-   ##
==========================================
+ Coverage   52.39%   53.66%   +1.26%     
==========================================
  Files         175      176       +1     
  Lines       13094    13127      +33     
==========================================
+ Hits         6861     7045     +184     
+ Misses       6233     6082     -151     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@abukosek abukosek force-pushed the andrej/feature/replace-ethabi branch 4 times, most recently from 9360abf to ec09fd7 Compare July 24, 2025 13:20
@abukosek abukosek marked this pull request as ready for review July 24, 2025 13:47
@kostko kostko requested a review from njelich July 29, 2025 14:04
@njelich
Copy link
Copy Markdown
Contributor

njelich commented Aug 5, 2025

@matevz The rewrite is more or less done, but currently blocked due to solabi not supporting packed encoding, which is needed to complete the signed_call rewrite. Should I add packed encoding support to solabi or use another library?

https://github.com/oasisprotocol/oasis-sdk/pull/2282/files#diff-21774ace7dc2170065f7b77e0f9a0e69cb1f26ab5773dc6b412530a5ac2504c0R196-R225

@njelich
Copy link
Copy Markdown
Contributor

njelich commented Aug 12, 2025

Need to check benchmarks for memory, binary size and speed between ethabi, solabi, alloy.

@njelich
Copy link
Copy Markdown
Contributor

njelich commented Aug 21, 2025

Reported on the benchmarks - solabi is about 2x faster and has a smaller binary than alloy, both on static and dynamic encoding tests. Proceeding to add support for packed encoding to solabi.

@abukosek
Copy link
Copy Markdown
Contributor Author

abukosek commented Sep 2, 2025

Generally looks OK, but please change the following:

  1. Sensibly join commits and use our style: https://github.com/oasisprotocol/oasis-core/blob/master/CONTRIBUTING.md#git-commit-messages
  2. There are some commented-out lines of code left -- either uncomment them or remove them (also extra whitespace).
  3. Run cargo fmt.
  4. Rebase to latest main.
  5. Re-run tests and linters on CI and make any adjustments to the code if necessary.

Also, it currently doesn't compile because of mbedtls errors (the version was probably bumped by accident, please undo that -- updating that should be done in a separate PR).

I'll do another review pass after these are done :)

@njelich njelich force-pushed the andrej/feature/replace-ethabi branch from 267c389 to 1de7998 Compare September 2, 2025 09:38
@njelich njelich requested a review from matevz as a code owner September 2, 2025 09:38
@njelich njelich force-pushed the andrej/feature/replace-ethabi branch from 1de7998 to 1fa1c5b Compare September 2, 2025 09:42
Comment thread examples/runtime-sdk/rofl-oracle-tdx-raw/src/main.rs Outdated
Comment thread runtime-sdk/modules/evm/Cargo.toml Outdated
@njelich njelich force-pushed the andrej/feature/replace-ethabi branch 6 times, most recently from 8a2b24a to ff379d5 Compare September 2, 2025 14:15
@njelich
Copy link
Copy Markdown
Contributor

njelich commented Sep 4, 2025

@abukosek Could you please take a look at the build issue to do with defguard_wireguard_rs package? Everything else is resolved.

@kostko
Copy link
Copy Markdown
Member

kostko commented Sep 4, 2025

The breakage is related to the issue I mentioned a few days ago, upstream is working on a fix due to incorrect semver bump.

See:

@njelich
Copy link
Copy Markdown
Contributor

njelich commented Sep 4, 2025

Upstream seems to have merged 10 minutes ago (DefGuard/wireguard-rs#95)

@njelich njelich force-pushed the andrej/feature/replace-ethabi branch 3 times, most recently from 19de2c5 to 3148263 Compare September 5, 2025 02:38
@njelich njelich force-pushed the andrej/feature/replace-ethabi branch from 3148263 to 02678ea Compare September 5, 2025 09:06
@njelich
Copy link
Copy Markdown
Contributor

njelich commented Sep 5, 2025

@abukosek Please reviw so I can merge - CI now passes.

Copy link
Copy Markdown
Contributor Author

@abukosek abukosek left a comment

Choose a reason for hiding this comment

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

The solabi packed encoding PR has been merged, so you can bump the solabi versions to the latest 0.3.0 :)

Comment thread runtime-sdk-macros/src/evm_derive.rs Outdated
Comment thread runtime-sdk/modules/evm/src/signed_call.rs Outdated
Comment thread runtime-sdk/modules/evm/src/signed_call.rs Outdated
Comment thread runtime-sdk/modules/evm/src/signed_call.rs
Comment thread runtime-sdk/modules/evm/src/types.rs Outdated
@njelich njelich force-pushed the andrej/feature/replace-ethabi branch from 02678ea to 836688c Compare September 8, 2025 13:54
@njelich
Copy link
Copy Markdown
Contributor

njelich commented Sep 8, 2025

Updated

Comment thread runtime-sdk/modules/evm/src/test.rs Outdated
@njelich njelich force-pushed the andrej/feature/replace-ethabi branch 3 times, most recently from d35311d to 49d7592 Compare September 9, 2025 07:37
@njelich
Copy link
Copy Markdown
Contributor

njelich commented Sep 9, 2025

@abukosek Linted, formatted, and reviewed for any remaining comments (leaving the comments inside functions describing functionality as normal ones), and removed unneeded prints.

@njelich njelich force-pushed the andrej/feature/replace-ethabi branch from 49d7592 to 0c5ce70 Compare September 9, 2025 10:21
Copy link
Copy Markdown
Contributor Author

@abukosek abukosek left a comment

Choose a reason for hiding this comment

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

As I've said previously, comments must end with periods to be consistent with the rest of our coding style :)

Comment thread runtime-sdk/modules/evm/src/types.rs Outdated
Comment thread runtime-sdk/modules/evm/src/types.rs Outdated
Comment thread runtime-sdk/modules/evm/src/types.rs Outdated
Comment thread runtime-sdk/modules/evm/src/types.rs Outdated
Comment thread runtime-sdk/modules/evm/src/types.rs Outdated
Comment thread runtime-sdk/modules/evm/src/types.rs Outdated
@njelich njelich force-pushed the andrej/feature/replace-ethabi branch from 0c5ce70 to 09d757e Compare September 9, 2025 13:25
Copy link
Copy Markdown
Contributor Author

@abukosek abukosek left a comment

Choose a reason for hiding this comment

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

Looks good now! 👍

GitHub says that "Pull request authors can’t approve their own pull request.", so someone else will have to approve it before it can be merged :D

@kostko kostko merged commit 5a4c9f6 into main Sep 9, 2025
31 checks passed
@kostko kostko deleted the andrej/feature/replace-ethabi branch September 9, 2025 14:26
github-actions Bot added a commit that referenced this pull request Sep 9, 2025
github-actions Bot added a commit that referenced this pull request Sep 9, 2025
…/andrej/feature/replace-ethabi

Replace ethabi 5a4c9f6
github-actions Bot added a commit to OasisUnofficial/oasis-sdk that referenced this pull request Sep 9, 2025
…sisprotocol/andrej/feature/replace-ethabi

Replace ethabi 5a4c9f6
github-actions Bot added a commit to OasisUnofficial/oasis-sdk that referenced this pull request Sep 9, 2025
…oasisprotocol/andrej/feature/replace-ethabi

Replace ethabi 5a4c9f6
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.

Replace ethabi with solabi

4 participants