-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(node_framework): Support StateKeeper in the framework #1043
Conversation
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 think there's some refactoring that could be left outside this PR, like injecting the ConnectionPool in MempoolFetcher. I know it's super annoying to make smaller PRs when CI is a PITA, but would still appreciate it when possible.
On documentation, I think it's the right call. Make sure there's a task to come back to it so it doesn't get lost.
core/node/node_framework/src/implementations/layers/fee_input.rs
Outdated
Show resolved
Hide resolved
/// The resource provider can be dynamic, however. For example, we can define a resource provider which may use | ||
/// different config load scheme (e.g. env variables / protobuf / yaml / toml), and which resources to provide | ||
/// (e.g. decide whether we need MempoolIO or ExternalIO depending on some config). | ||
#[derive(Debug)] |
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.
not debug anymore?
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.
Good catch, will fix in the next PR!
@@ -7,7 +7,7 @@ use crate::service::StopReceiver; | |||
|
|||
/// A task implementation. | |||
#[async_trait::async_trait] | |||
pub trait Task: 'static + Send + Sync { |
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.
why not Sync
anymore?
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.
It doesn't have to be Sync
in fact (no task is accessed from two thread simultaneously), so it was an incorrect bound.
🤖 I have created a release *beep* *boop* --- ## [20.7.0](core-v20.6.0...core-v20.7.0) (2024-02-16) ### Features * Add input field to CallRequest ([#1069](#1069)) ([5087121](5087121)) * **api:** Remove unused and obsolete token info ([#1071](#1071)) ([e920897](e920897)) * Better errors for JsonRPC calls ([#1002](#1002)) ([079f999](079f999)) * **commitment:** Commitment component ([#1024](#1024)) ([60305ba](60305ba)) * **en:** Make snapshots applier resilient and process storage log chunks in parallel ([#1036](#1036)) ([805218c](805218c)) * **node_framework:** Resources and layers for ETH clients ([#1074](#1074)) ([776337a](776337a)) * **node_framework:** Support StateKeeper in the framework ([#1043](#1043)) ([a80fff2](a80fff2)) ### Bug Fixes * **api:** Return on duplicate earlier ([#1059](#1059)) ([cfa5701](cfa5701)) * **contract-verifier:** Use optimizer mode in solidity-single-file verification ([#1079](#1079)) ([fdab638](fdab638)) * Token distribution ([#1051](#1051)) ([bd63b3a](bd63b3a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
What ❔
ResourceProvider
abstraction. State keeper task finally showed that it brings more problems than solutions. Main implication: now ordering of layers matters ("base" layers should go first), but hopefully it shouldn't cause too much problems.PoolsLayer
that was previously handled byResourceProvider
.SequencerFeeInputLayer
andObjectStoreLayer
.Box<smth>
as a non-shareable resource, like IO).StateKeeperLayer
,MempoolIOLayer
, andMainNodeBatchExecutorBuilderLayer
).Current "external" point of view for the framework: