CosmWasm / cw-storage-plus

Storage abstractions for CosmWasm smart contracts
Apache License 2.0
37 stars 27 forks source link

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

Open apollo-sturdy opened 4 months ago

apollo-sturdy commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Hi!

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.