SnapshotMap height loading discrepancy

We use SnapshotMap a lot over at DAO DAO, and there's one aspect of it that has always bothered me. I think it leads to pretty confusing behavior and is likely to cause bugs, potentially security issues, if not deeply understood by the developers using it.

In my understanding, when a new value V_new is saved at height n, a snapshot is created at height n that contains the current value V_old in a field called old, and the value in the primary map is overridden with V_new. load and may_load read directly from the primary map, finding V_new, behaving as expected. But may_load_at_height actually returns V_old when called with the height at which V_new was saved.

Thus, may_load differs from may_load_at_height if used in the same block right after a value is stored. IMO, one would assume that calling save at the current height followed by may_load_at_height at the current height would retrieve the value just saved, but that is not the case.

The only justification I could see for this behavior is that SnapshotMap intends to be deterministic for the whole block, always returning the value that existed at the very beginning of the block. Thus, if another contract queries a contract that uses a SnapshotMap during a block where the map is being updated, it will always get the same value, regardless of the ordering of transactions in the block. Though I'm not totally convinced by this argument as one already cannot expect deterministic ordering when interacting with other contracts, and other state storage exists besides SnapshotMap. And I'm not sure if this benefit is worth the tradeoff of less intuitive security critical code.

Is there another reason it was designed this way? Please convince me this is necessary 😁

There are a few ways I think this could be fixed pretty easily, though I don't have full knowledge of this monorepo, and maybe these would break other things. Let me know what you think.

In write_change https://github.com/CosmWasm/cw-storage-plus/blob/cac9687e29579c61eeacffafc131614c9f43baaa/src/snapshot/map.rs#L117-L126 use height - 1 instead of height, since that is the last block at which the old value is accurate.
In may_load_at_height https://github.com/CosmWasm/cw-storage-plus/blob/cac9687e29579c61eeacffafc131614c9f43baaa/src/snapshot/map.rs#L154-L170 use height + 1 when calling self.snapshots.may_load_at_height, since it will try to find the next change after the requested height, whose old value would be the correct value for height.
In Snapshot https://github.com/CosmWasm/cw-storage-plus/blob/cac9687e29579c61eeacffafc131614c9f43baaa/src/snapshot/mod.rs#L151-L180 change the starting lower bound from inclusive to exclusive when looking in the changelog for the old value.

As far as I can tell, 2 and 3 above are identical, except 3 may fix the problem in other places if Snapshot is used the same way.

CosmWasm / cw-storage-plus

SnapshotMap height loading discrepancy #90