EXP-83 Create basic scaffolding for bridge core impl

[Rajil1213]

Description

This PR adds the foundational structures for the core bridge implementation as it is supposed to work in devnet. This includes the core bridge types, traits and some very basic implementation outlines (see the operator::Execute trait). Much of this is still a work in progress and far from perfect but should be a good enough starting point for the rest of the implementation.

The following traits will be covered in separate PRs:

• Bridge client database traits for checkpointing and signature aggregation (EXP-158)
• Bridge transaction assembly (EXP-128)

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactor

Checklist

• I have performed a self-review of my code.
• I have commented my code where necessary.
• I have updated the documentation if needed.
• My changes do not introduce new warnings.
• I have added tests that prove my changes are effective or that my feature works.
• New and existing tests pass with my changes.

Related Issues

EXP-83

Notes for Reviewers

A lot of files have been changed, most of which are new ones. So, it should be okay to squash-merge this PR so as to avoid rebasing hell for others.

[codecov]

Codecov Report

Attention: Patch coverage is 36.74497% with 377 lines in your changes missing coverage. Please review.

Project coverage is 57.43%. Comparing base (61363b5) to head (24fd3df).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
bin/bridge-client/src/rpc_server.rs	0.00%	57 Missing ⚠️
crates/bridge-sig-manager/src/state.rs	0.00%	49 Missing ⚠️
crates/primitives/src/bridge.rs	0.00%	43 Missing ⚠️
crates/primitives/src/l1.rs	84.55%	40 Missing ⚠️
crates/bridge-tx-builder/src/builder.rs	0.00%	29 Missing ⚠️
crates/bridge-tx-builder/src/deposit.rs	0.00%	28 Missing ⚠️
bin/bridge-client/src/main.rs	0.00%	25 Missing ⚠️
...ates/bridge-exec/src/withdrawal_handler/handler.rs	0.00%	15 Missing ⚠️
crates/bridge-sig-manager/src/signature.rs	0.00%	13 Missing ⚠️
bin/bridge-client/src/args.rs	0.00%	12 Missing ⚠️
... and 20 more

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   58.20%   57.43%   -0.78%     
==========================================
  Files         133      157      +24     
  Lines       14404    14977     +573     
==========================================
+ Hits         8384     8602     +218     
- Misses       6020     6375     +355     

Files with missing lines	Coverage Δ
crates/rpc/types/src/types.rs	0.00% <ø> (ø)
bin/bridge-client/src/errors.rs	0.00% <0.00%> (ø)
crates/bridge-exec/src/book_keeping/errors.rs	0.00% <0.00%> (ø)
crates/bridge-exec/src/config/errors.rs	0.00% <0.00%> (ø)
crates/bridge-exec/src/deposit_handler/errors.rs	0.00% <0.00%> (ø)
...rates/bridge-exec/src/withdrawal_handler/errors.rs	0.00% <0.00%> (ø)
crates/primitives/src/buf.rs	78.57% <0.00%> (-0.48%)	⬇️
crates/primitives/src/errors.rs	0.00% <0.00%> (ø)
crates/rpc/api/src/lib.rs	0.00% <0.00%> (ø)
crates/state/src/bridge_state.rs	15.38% <0.00%> (+0.16%)	⬆️
... and 21 more

... and 2 files with indirect coverage changes

[delbonis]

I have some concerns about this architecture pushing impl details into this deeply nested trait hierarchy.

What this is actually doing is making it a requirement that all of the logic is bottled into a single type and we're implicitly exposing implementation details by making the traits dependent on each other. It also means that it's hard to test just a subset of the functionality of part of the ball of state, because then you'd have to reimplement that functionality on another type that only implements the traits that you're testing. This is some really scary coupling because it's also a lot more implicit and the components are all flattened out into a one type.

Especially, for example, if you wanted write unit tests for the middle of 3 layers of the traits, it would be fairly straightforward to peel off the top layer of the impl, but less obvious how to abstract out the underlying layer. Each of these traits should be split out into its own system so it can be tested in isolation independently of any others, and (if need be) mocking out its dependencies. The low-level logic for signing transactions should be outside of this trait system, so we can test it directly without the need for mocking anything.

I would go back to the drawing board on the internal architecture of the operator client. Start from the bottom up with the core loop of polling/tracking for duties, only implement things concretely and then abstract things out. When it comes time to do transaction assembly, build the transaction assembly first and then figure out how to integrate it. I have some design guidelines in a few of the bridge-related tickets that might help frame it better.

[Rajil1213]

@delbonis

This is some really scary coupling because it's also a lot more implicit and the components are all flattened out into a one type.

Yeah, there are distinct functionalities here such as the deposit handling is very different from the withdrawal handling. So having separate traits made sense in that regard. But ultimately, it is the bridge client that performs all the duties which means all the traits are coupled together and that is just bad.

Each of these traits should be split out into its own system so it can be tested in isolation independently of any others, and (if need be) mocking out its dependencies

How would this work? Are you saying, there'd be distinct types that implement each of these traits? A DepositHandler struct that implements the HandleDeposit trait? I could make that work but it would need to share at least the rpc client and a database handle with the WithdrawalHandler.

The low-level logic for signing transactions should be outside of this trait system, so we can test it directly without the need for mocking anything.

Yeah this is the case even now. The express-bridge-txm crate exists at the lowest-level of the crate-hierarachy. It depends only on the express-primitives crate and all the other bridge crates depend on it.

I would go back to the drawing board on the internal architecture of the operator client. Start from the bottom up with the core loop of polling/tracking for duties, only implement things concretely and then abstract things out. When it comes time to do transaction assembly, build the transaction assembly first and then figure out how to integrate it. I have some design guidelines in a few of the bridge-related tickets that might help frame it better.

The approach I took was to go the other way around -- start with the high-level end-to-end structure and then, peeling back the layers to the point where there is a function that needs to be implemented later. Of course, things are likely to change when we actually start implementing the guts of the client. Do you think the scaffolding itself should change beyond refactorings discussed above? For example, I see the individual deposit and withdrawal traits sticking around even though how they compose together might change.

[delbonis]

Are you saying, there'd be distinct types that implement each of these traits? A DepositHandler struct that implements the HandleDeposit trait? I could make that work but it would need to share at least the rpc client and a database handle with the WithdrawalHandler.

Yes this is how this is normally done in Rust and it's how we do it in the main rollup client codebase.

start with the high-level end-to-end structure and then, peeling back the layers to the point where there is a function that needs to be implemented later. Of course, things are likely to change when we actually start implementing the guts of the client.

This issue with this is we don't know if we've taken the right approach with some until we've implemented it, and already we've baked in a fairly rigid relationship between the different components. If you can even really call them separate components, since it'd all have to be on of a single type.

Do you think the scaffolding itself should change beyond refactorings discussed above? For example, I see the individual deposit and withdrawal traits sticking around even though how they compose together might change.

Will DM for further discussion on these points.

[delbonis]

I know this PR is somewhat on hold pending lower-level impls but I have some more comments since there's been some iteration on it.

[bewakes]

Just a few queries.

[Rajil1213]

Okay, I've made a final round of changes on this PR. Also brought in a few traits and primitives from #269 (some of it might change but nothing a rebase cannot fix later). All the CI checks are passing and it's up to date with master.

[storopoli]

I think we can merge this and start working on the fresh tickets that are in this sprint and unlocked by this PR.

[storopoli]

ACK 24fd3df
But, please squash merge these 60 commits