Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make Snapshot take a strategy trait so that users can write their own strategies #82

Open
wants to merge 6 commits into
base: release/1.2
Choose a base branch
from

Conversation

apollo-sturdy
Copy link

This PR adds a SnapshotStrategy trait and adds a generic to Snapshot, SnapshotMap, SnapshotItem, and IndexedSnapshotMap. It also implements the SnapshotStrategy trait for the Strategy enum.

This PR also adds a IntervalStrategy struct which is a strategy that only stores data to the changelog if at least a certain number of blocks has passed.

I tried my best to make these changes non-breaking and I think I succeeded. However, I think there are more changes that could be make to simplify and improve Snapshot. For example, I found the "checkpointing" and the fn assert_checkpointed quite confusing and it seems this is only used in the Strategy::Selected variant. I couldn't figure out a way to move this logic into this variant without making breaking changes.

I also found it a bit unintuitive that in SnapshotMap, data added to the changelog use the current height and not the height at which they were originally saved to the primary Map. For the current purposes that I want to use SnapshotMap for it would make sense to use the height at which the data was originally saved, so that the changelog becomes a record of history rather than having a delay in the heights. Would you be interested in me adding an alternative implementation that accomplishes this? Trying to modify the current implementation would cause breaking changes.

@apollo-sturdy
Copy link
Author

I just realized that since Snapshot is pub(crate) I'm not able to make my own "HistoryMap" that archives data with the height at which they were originally saved rather than at the current height. Is there a reason to keep Snapshot as pub(crate) rather than pub?

@uint
Copy link
Contributor

uint commented Jul 4, 2024

Hi!

  • I suspect we can make Snapshot public if it helps devs create their abstractions.
  • I think the overall point of this PR is to make things that should be open to extension, well... open to extension. I'm pretty happy with that.
  • In general, we don't particularly want to add features to this library, but I think IntervalSnapshot sounds useful enough.

I do appreciate the non-API-breaking way this was implemented.

Is there a reason you're targetting the release/1.2 branch rather than just main? If you need a backport, I think we should start with main anyway and then backport.

use super::{ChangeSet, SnapshotStrategy};

/// A SnapshotStrategy that takes a snapshot only if at least the specified interval has passed.
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]

clippy complains here

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants