-
Notifications
You must be signed in to change notification settings - Fork 5
fix: remove create_tables_with_snapshot_and_commitment and CommitmentCreationCmd::FromSnapshot from pallet tables
#82
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
base: main
Are you sure you want to change the base?
Conversation
1.31.1Bug Fixes
|
| /// From a preexisting commitment | ||
| FromSnapshot(SnapshotUrl, TableCommitmentBytesPerCommitmentScheme), | ||
| /// An empty commitment | ||
| Empty(CommitmentSchemeFlags), |
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.
@tlovell-sxt Shall we just remove this enum? Do we plan to add more variants?
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 kinda think we should just remove the enum. Technically either way is pretty breaking, but I believe lots of things around this will be changing downstream soon anyway.
…entCreationCmd::FromSnapshot ` from pallet tables
|
|
||
| /// Clear schemas and tables from chain state for all namespaces and identifiers | ||
| #[pallet::call_index(3)] | ||
| #[pallet::call_index(1)] |
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 don't think there's really a need to adjust the call indices, I think they're allowed to have gaps and it's less breaking to let calls sit on their indices
| #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, TypeInfo, MaxEncodedLen)] | ||
| pub enum CommitmentCreationCmd { | ||
| /// From a preexisting commitment | ||
| FromSnapshot(SnapshotUrl, TableCommitmentBytesPerCommitmentScheme), |
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.
Can SnapshotUrl be removed?
| /// From a preexisting commitment | ||
| FromSnapshot(SnapshotUrl, TableCommitmentBytesPerCommitmentScheme), | ||
| /// An empty commitment | ||
| Empty(CommitmentSchemeFlags), |
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 kinda think we should just remove the enum. Technically either way is pretty breaking, but I believe lots of things around this will be changing downstream soon anyway.
Rationale for this change
We no longer create tables with snapshot.