feat: implement unified asset delta#3038
Conversation
e9b41ad to
2cccbef
Compare
ddc69d4 to
09b9867
Compare
|
FYI: Updated the PR description to describe the new approach. |
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - but they are mostly non-blocking.
One thing that would be good to find out is how the number of transaction cycles has changed after this PR. Probably worth re-running the benchmarks for this.
Sure! Essentially, the new code takes ~300 cycles more in both the single and two P2ID consumption case. So it does not seem to scale with the number of assets. (Edit: Actually, the benchmarks I'm looking at compute the delta twice, so the diff shows as doubled - so comparing prev and new |
mmagician
left a comment
There was a problem hiding this comment.
LGTM with minor nits and some questions, but nothing blocking so feel free to merge when ready!
Unifies the fungible and non-fungible asset delta tracking inside the kernel into a single asset delta.
add_asset/remove_assetroute every asset (regardless of composition) into oneaccount_delta::update_assetprocedure that combines the incoming delta with the current delta. The main benefit is that we now have one logic that handles both fungible and non-fungible assets, and should also support custom assets (as long as they implement certain APIs like merge, split and compare).The initial idea for the asset delta is described in the OP in #2630, but was changed to a different approach during the PR review.
The delta now tracks a link map with entries:
Similarly, the delta is computed by computing the diff between init and final. This requires that we can order any assets that do NOT use
AssetComposition::None, which we require withasset::lt. That way, we can figure out which asset is "greater" and computediff = greater - lesseras well as whether it was removed (final less than init) or added (final greater than init).Only
asset::ltis required rather than a more generalasset::compare. The main reason is that equality tests are handled by the kernel itself usingword::test_eq, which would only leaveltandgtto be implemented bycompare, and to avoid explaining this subtlety, onlyltis required. Since this is all kernel-internal for now, we can easily make changes to this in the future, before we formalize support for custom assets.The delta commits to the list of added assets, followed by a trailer and then the list of removed assets, followed by a trailer (Option 2 mentioned in #2630 but with an optimization).
Look-ahead: The asset patch commitment can iterate the same asset list link map to figure out if an asset has changed (
init != final) and commit to the final asset value.Follow-up:
AccountVaultDelta(can only be done after tx kernel commits to patches instead of deltas).account_deltamodule toaccount_patchand similar things. Likely makes more sense going forward.closes #2630