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

chore(eth): refactor eth API module into separate pieces in new pkg #12796

Merged
merged 3 commits into from
Dec 20, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Dec 19, 2024

As I start on the /v2 API the eth module is my first hurdle: I was /v1 to remain as it is now, but /v2 to have the same Eth endpoints but be F3 aware, so "finalized" and "safe" get adjusted according to F3. Otherwise they should be the same. Because almost everything is in a single eth.go, it makes it a bit hard to do a nice pluggable solution where I can reuse most of the code and just have replacements for a resolver. So I decided to rip apart the eth.go monolith into smaller components, in their own package, with interfaces for their dependencies rather than the concrete types.

There's very little change in the code even though the diff is big, it's mostly just moving things around. I took a few opportunities to clean up some awkward messes, and I was forced to do some new mildly awkward new things.

  • github.com/filecoin-project/lotus/node/impl/eth is a new package, with what used to be in github.com/filecoin-project/lotus/node/impl/full/eth*.go but logically split up.
  • The new eth package requires on fewer pieces of the rest of the codebase, in particular interfaces have been introduced for the main functional dependencies like ChainStore and StateManager.
  • github.com/filecoin-project/lotus/node/impl/gasutils contains some components that were in node/impl/full/gas.go but are shared between Filecoin and Eth APIs.
  • The construction and startup utilities in node/modules/ethmodule.go are now in node/modules/eth.go but have been trimmed because we need to do less manual wiring now.
  • There's some pieces I haven't touched, like the filter stuff and subscription manager.
  • Made consistent use of xerrors in the new eth package.

node/impl/eth/events.go Fixed Show fixed Hide fixed
node/impl/eth/events.go Fixed Show fixed Hide fixed
@rvagg rvagg force-pushed the rvagg/ethmodule-refactor branch 2 times, most recently from 889a696 to 0d22219 Compare December 19, 2024 08:39
@rvagg rvagg added the skip/changelog This change does not require CHANGELOG.md update label Dec 19, 2024
@rvagg rvagg marked this pull request as ready for review December 19, 2024 23:15
@rvagg rvagg requested a review from Stebalien December 19, 2024 23:16
node/modules/eth.go Outdated Show resolved Hide resolved
node/modules/eth.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member Author

rvagg commented Dec 20, 2024

Additional fixes applied after a sync zoom chat with @Stebalien about this:

  • Clarified naming - everything that defines an external API has an API suffix in the name, and I've removed that from some interfaces I was using to define internal things (like ChainStore). It's much clearer now when you view it through the lens of what's intended for RPC and what's not.
  • Context naming - lctx for the lifecycle context on init.
  • Added some docs around the GatewayEthSend special-case. But also, we might look at alternative ways to improve this in the future, perhaps with some more creative/newer fx use.
  • Misc cleanups from things I noticed while walking back through the code, again.

@rvagg rvagg force-pushed the rvagg/ethmodule-refactor branch from 8998024 to 27a1b51 Compare December 20, 2024 08:25
@rvagg rvagg removed the skip/changelog This change does not require CHANGELOG.md update label Dec 20, 2024
@rvagg rvagg enabled auto-merge (squash) December 20, 2024 08:26
@rvagg rvagg merged commit edfa817 into master Dec 20, 2024
84 checks passed
@rvagg rvagg deleted the rvagg/ethmodule-refactor branch December 20, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

2 participants