Refactored delta's vault and storage to use maps #822
Conversation
polydez
force-pushed
the
polydez-delta-refactorings
branch
from
dd9b152 to
7180518
Compare
There was a problem hiding this comment.
Looks good! Thank you! Not a full review - but I left some comments inline.
One general comment is that I think we should not impose limits on individual deltas (e.g., how many assets can be added or removed etc.). Instead, in a different PR, we can add a way to estimate the size of the delta and impose a limit on that.
| 1 => NonFungibleDeltaAction::Add, | ||
| -1 => NonFungibleDeltaAction::Remove, | ||
| _ => unreachable!("non-fungible asset amount must be 1 or -1"), |
There was a problem hiding this comment.
Let's change the struct definition to use the enum instead
non_fungible_assets: BTreeMap<Digest, NonFungibleDeltaAction>,There was a problem hiding this comment.
I actually think there is currently a bug in add and remove asset where non-fungibles can be doubly added or removed. This change would prevent that - and I believe creating a private fn update_non_fungible_asset would be best.
Unless its allowed that delta can have sequential removals so long as in the end it all balances out? i.e. is this valid for a non-fungible delta aggregation:
+1 1
+1 2
+1 3
+1 4
-1 3
-1 2
-1 1 == valid
objects/src/assets/vault.rs
Outdated
| } | ||
|
|
||
| for (&asset, &action) in delta.non_fungible() { | ||
| // # SAFETY: the asset must be a valid word representation of a non-fungible asset |
There was a problem hiding this comment.
The safety comment should also explain why its guaranteed safe (currently it just repeats the unsafe comment of the called fn).
| // # SAFETY: the asset must be a valid word representation of a non-fungible asset | |
| // SAFETY: the asset must be a valid word representation of a non-fungible asset |
There was a problem hiding this comment.
Looking at this further -- is this guaranteed by the delta? If yes, should we not change the delta's definition to use NonFungibleAsset instead of a raw Digest?
There was a problem hiding this comment.
I don't see another good solution, because we use add method of fungible asset in order to update its amount. We could change only processing of non-fungible assets, but I don't think it would be a good idea.
| pub const fn new(slots: BTreeMap<u8, Word>, maps: BTreeMap<u8, StorageMapDelta>) -> Self { | ||
| Self { slots, maps } | ||
| } |
There was a problem hiding this comment.
Can we combine AccountStorageDelta and AccountStorageDeltaTracker? They seem like the exact same thing.
# Conflicts: # CHANGELOG.md # miden-tx/src/tests/mod.rs
* refactor: use maps in storage and vault deltas * docs: update CHANGELOG.md * refactor: address review comments * tests: fix compilation errors * refactor: address review comments * refactor: address review comments
Resolves #798
In this PR I refactored account's delta storage and vault to use maps instead of vectors. This simplifies all workflows a bit. I also updated all algorithms of building, validating, merging and applying deltas. In order to minimize affection to tests, creation by using updated/cleared items is still supported.