-
Notifications
You must be signed in to change notification settings - Fork 16
feat: allow for user defined checkpoint metadata #78
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
Conversation
bf0e1ed to
de7f40f
Compare
|
Looks great! Let's just tweak the public API a little bit. |
julio4
left a comment
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.
This is good!
I generally think that we should be careful about changing API and that moving to a builder pattern should be discussed separately.
Also, we should try to not overload too much method and try to extract metadata from within checkpoint when possible.
And lastly, we attach metadata to checkpoint but this is basically a state that is passed across checkpoint to give more context required by some platform. I think maybe metadata is not exactly the correct name, maybe something like BuildingContext could be more relevant if I understood this correctly.
What do you think?
|
Now that we will have |
de7f40f to
67f3802
Compare
julio4
left a comment
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.
lgtm, be sure to rename all metadata to context
src/payload/exec.rs
Outdated
| Self::Bundle(bundle) => { | ||
| Self::execute_bundle(bundle, block, db, checkpoint_context) | ||
| } | ||
| Self::Transaction(tx) => Self::execute_transaction(tx, block, db) |
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.
We can also add context to execute_transaction even if it's not used yet
Add an associated type to
Platformto hold state that is passed around with checkpoints. This makes it easier to write features like flashblocks, since multiple steps need to access the flashblock number and so we currently define state outside the pipeline and share it with those steps. This PR also makes it possible to access this state during bundle filtering, so we will can filter on flashblock numbers.Checkpoint metadata can be accessed via
checkpoint.metadata()and can be updated likecheckpoint.execute(tx).metadata(new_metadata).apply(). If metadata isn't set for the next checkpoint then the current checkpoint's metadata is cloned to the next checkpoint.I refactored the checkpoint construction api because we want to be able to optionally set tags and/or metadata. Instead of
apply(tx)orbarrier()returning the next checkpoint, theexecute(tx)andbarrier()methods now returnCheckpointBuilderinstances which has anapply()method to create the next checkpoint, along withtagandmetadatasetter functions. This results in more verbose code but it seems like the clearest way to enable metadata and tags.