chore: refactor AccountVaultDelta to track generic assets - #3485
Conversation
e31bd2c to
ba068da
Compare
zeapoz
left a comment
There was a problem hiding this comment.
Looks good! No major complaints, but I think we could return the previous (overwritten) value and pass it down the call stack
mmagician
left a comment
There was a problem hiding this comment.
LGTM, with one point to discuss: we used to have normalization for non-zero fungible assets as:
// Only track non-zero amounts.
if delta != 0 {
entry.insert(delta);
}which is now gone.
This means "no net change" & "empty delta" used to be the same, and with this PR that's no longer the case.
Also the kernel only commits to entires in the map, so before building a delta containing an Add(amount=0) would still match the kernel-computed commitment. Now, Rust side will contain an entry while the kernel can never emit such a delta.
Good point, thanks for bringing this up! I think the need for this extra logic went away with #2630. For example, consider the case of asset 1 in this test setup: protocol/crates/miden-testing/src/kernel_tests/tx/test_account_update.rs Lines 351 to 359 in 134cc1f The tx kernel tracks In other words, the special Rust logic is no longer necessary because the consistency between the in-kernel delta and the host delta is guaranteed by constructing the host delta from the in-kernel delta with normalization in mind. |
Summary
AccountVaultDeltano longer splits vault changes into a fungible and a non-fungible half. It now stores a single map of genericAssetDeltaentries keyed byAssetId, which matches how the transaction kernel already reports vault changes.Changes
AccountVaultDeltais nowBTreeMap<AssetId, AssetDelta>withAssetDeltamoving tomiden-protocol.Commitment format is unchanged.
read_fromnow routes through the validatingnewconstructor, so a crafted byte stream that lists the same asset in both the added and the removed section is rejected.closes #3175