-
Notifications
You must be signed in to change notification settings - Fork 336
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(ICP-Ledger): FI-1442: migrate ledger blocks to stable structures #3836
base: master
Are you sure you want to change the base?
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.
LGTM, thanks @maciejdfinity!
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.
Left a few comments/suggestions: nothing major => all non-blocking.
/// Stores a chain of transactions with their metadata | ||
#[derive(Debug, Deserialize, Serialize)] | ||
#[serde(bound = "BD: Serialize, for<'a> BD: Deserialize<'a>")] |
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.
Since I don't need the custom bounds for BD anymore I removed this line completely. Before the ICRC V5 PR, this line used to be #[serde(bound = "")]
: 0d96610#diff-1122f1a694a6ec31534fa34a1f861a31aeaaf0cdcc8d01406dd39a480f2e7bb1L16 @mbjorkqvist @mraszyk do you know if I need the empty bounds here?
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 wouldn't expect empty bounds to make a difference, but I don't know with certainty.
This version migrates blocks stored in the ledger to stable structures. This allows the ledger to store more blocks and reduces the need for archiving. The archiving is still performed but its parameters (e.g.
trigger_threshold
) can be adjusted. The migration is performed using timers, see #3314 description for more details.