-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat(provider): add cached DA oracles for bedrock/nitro #842
Conversation
48a3e6e
to
012c946
Compare
feat(provider): add a local bedrock DA provider feat(provider): add a cached arbitrum nitro da gas oracle
012c946
to
0baf49c
Compare
0baf49c
to
a89bd4f
Compare
Will add more tests in this ticket: #845 Want to get the mempool usage PR up quickly. |
Ok(DAGasBlockData::Empty) | ||
} | ||
|
||
async fn uo_data( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I get the idea of this method and above are for impl the trait. could we move those impl in the trait definition so they by default return empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I typically don't like default method implementations, especially when they can cause panics, typically means the interface is wrong.
I think the better way to fix this would be to move the 3 new methods into a new trait: DAGasOracleSync
so that we can be sure they only get called if they're implemented.
Then instead of using the da_tracking_enabled
bool we pass an Option<DAGasOracleSync>
to the pool/builder which uses it if populated.
For the sake of time I'm going to add a ticket to clean this up. Might move it to v0.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposed Changes