feat: introduce op-angstrom, genericize repo for OP#569
Open
feat: introduce op-angstrom, genericize repo for OP#569
op-angstrom, genericize repo for OP#569Conversation
mempirate
force-pushed
the
feat/rollup-compat
branch
from
32fc110 to
1d644e0
Compare
- Remove default StromNetworkHandle parameter and angstrom-network dependency - Fix unused mode field by using PhantomData<M> pattern - Update call sites to use mode-specific constructors (new_consensus/new_rollup) - Rename PoolStromMessage to PoolNetworkMessage for generic naming - Simplify NetworkHandle trait to reduce tokio-stream dependency - Reorder generic parameters (NH before M) to support defaults properly This achieves clean extraction with proper decoupling while maintaining the benefits of the type-state pattern and static dispatch. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit addresses all the key points from the PR review to improve the pool-manager crate extraction and type-state pattern implementation: ## API Simplification - Remove public `PoolManagerBuilder::new()` method that took unused `_mode` parameter - Keep only mode-specific constructors (`new_consensus` and `new_rollup`) - This enforces static dispatch by construction and prevents accidental usage ## Ergonomic Improvements - Add convenient type aliases in `pool-manager/src/lib.rs`: - `ConsensusPoolManager<V, GS, NH>` - `RollupPoolManager<V, GS, NH>` - `ConsensusPoolManagerBuilder<V, GS, NH>` - `RollupPoolManagerBuilder<V, GS, NH>` - Reduces verbosity at call sites and improves developer experience ## Type System Improvements - Add `M: PoolManagerMode` bound directly on struct declarations - Remove unnecessary `+ Unpin` bounds on `NH: NetworkHandle` where not needed - Use `PhantomData<fn() -> M>` instead of `PhantomData<M>` for better dropck behavior - Makes intent clearer and widens compatibility ## Dependency Cleanup - Remove unused `tokio-stream` dependency from `crates/types/Cargo.toml` - Reduces the dependency footprint of the types crate ## Documentation - Update stale comments about type alias locations - Improve inline documentation for clarity All changes maintain backward compatibility while significantly improving the API design and developer experience. The type-state pattern now enforces correct usage through the type system more effectively. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
chore(cli): add sequencer endpoint configuration for rollup mode
chore: remove outdated todo
chore: add amm quoter tests
chore: extract and test helpers
chore: test op_angstrom cli
Will-Smith11
reviewed
Oct 9, 2025
Comment on lines
+26
to
+27
| #[clap(short, long, num_args(1..=5), require_equals = true)] | ||
| pub normal_nodes: Option<Vec<String>>, |
Collaborator
There was a problem hiding this comment.
why have we changed the type here from
#[clap(short, long, num_args(0..=5), require_equals = true, default_values = ETH_DEFAULT_RPC)]
pub normal_nodes: Vec<String>,
this was setup as it allows us to enable and disable sending from the mempool aswell have having overrides.
Comment on lines
+29
to
+35
| // TODO(mempirate): Is this a good value? | ||
| /// The factor by which the block time is multiplied to get the bid aggregation | ||
| /// deadline. = 80% of the block time. | ||
| /// | ||
| /// NOTE: On lower block times (like Flashblocks, we might have to decrease this | ||
| /// to account for transaction inclusion latency (network + processing)). | ||
| const BID_AGGREGATION_DEADLINE_FACTOR: f64 = 0.8; |
Collaborator
There was a problem hiding this comment.
If you look on the main branch, we have a slot clock impl that we use to offset message time with slot clock. Ideally this is used instead of a constant mutliple because it doesn't account for sequencer lag
Comment on lines
+159
to
+172
| fn on_blockchain_state( | ||
| &mut self, | ||
| notification: CanonStateNotification<OpPrimitives>, | ||
| waker: Waker | ||
| ) { | ||
| let new_block = notification.tip(); | ||
| let number = new_block.number(); | ||
|
|
||
| tracing::info!(?number, "New blockchain state"); | ||
| self.current_height = number; | ||
|
|
||
| // Reset the timeout. | ||
| self.state | ||
| .reset(self.block_time.mul_f64(BID_AGGREGATION_DEADLINE_FACTOR)); |
Collaborator
There was a problem hiding this comment.
needs to be updated to slot clock implimentation
Comment on lines
+230
to
+259
| tracing::trace!(height = self.current_height, "Building bundle"); | ||
| let bundle = match AngstromBundle::from_pool_solutions( | ||
| pool_solutions, | ||
| all_orders, | ||
| &pool_snapshots, | ||
| details | ||
| ) { | ||
| Ok(bundle) => bundle, | ||
| Err(e) => { | ||
| tracing::error!( | ||
| ?e, | ||
| height = self.current_height, | ||
| "Error building bundle! No bundle will be submitted" | ||
| ); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| // Reset the state to waiting. State transition cycle is now complete, ready for | ||
| // the next block to start the next cycle. | ||
| self.state = DriverState::Waiting; | ||
|
|
||
| let target_block = self.current_height + 1; | ||
| let provider = self.provider.clone(); | ||
| let signer = self.signer.clone(); | ||
| let height = self.current_height; | ||
|
|
||
| // NOTE: We can just spawn the submission here. We don't need to store the | ||
| // result, since in rollup mode we don't do anything with it. | ||
| tokio::spawn(async move { |
Collaborator
There was a problem hiding this comment.
I don't see anything here for unlock attestations
| fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { | ||
| let this = self.get_mut(); | ||
|
|
||
| use crate::common::PoolManagerCommon; |
| let this = self.get_mut(); | ||
|
|
||
| use crate::common::PoolManagerCommon; | ||
| PoolManagerCommon::poll_eth_and_pool(this, cx); |
Collaborator
There was a problem hiding this comment.
why is it being invoked this way instead of just calling on this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reasoning is documented in an ADR here: https://github.com/SorellaLabs/angstrom/blob/feat/rollup-compat/docs/adr/rollup.md
This will be a temporary feature branch until we get it all to compile. Related PRs will be based off of this branch.
@Will-Smith11 let me know if this looks good, otherwise we can brainstorm a different approach