Replace Balances.PutWithCreatable()

[tolikzinovyev]

Summary

Delete Balances.PutWithCreatable() and add new functions that notify the COW that a creatable was created or deleted. Coming up next is similar two functions for asset/application local state, needed by indexer.

[algorandskiy]

What it the rationale of replacing one function with another? Why not Put(addr, ad, ctype, cidx)?

[tolikzinovyev]

With indexer needs, there will be five scenarios:

• update account data
• update account data + create creatable
• update account data + delete creatable
• update account data + opt in creatable
• update account data + opt out creatable

Rather than blow up the interface of PutWithCreatable() I propose we have separate functions. This is not ideal, so I'm open to ideas.

I don't understand your suggestion though. What would Put(addr, ad, ctype, cidx) do?

[codecov-commenter]

Codecov Report

Merging #2638 (2235976) into master (916154b) will decrease coverage by 0.02%.
The diff coverage is 55.17%.

@@            Coverage Diff             @@
##           master    #2638      +/-   ##
==========================================
- Coverage   47.00%   46.97%   -0.03%     
==========================================
  Files         348      348              
  Lines       55717    55712       -5     
==========================================
- Hits        26188    26170      -18     
- Misses      26588    26595       +7     
- Partials     2941     2947       +6     

Impacted Files	Coverage Δ
ledger/apply/asset.go	19.78% <0.00%> (+0.10%)	⬆️
ledger/cow.go	89.28% <ø> (-1.04%)	⬇️
ledger/apply/application.go	75.37% <50.00%> (-2.13%)	⬇️
ledger/eval.go	76.88% <100.00%> (+0.20%)	⬆️
ledger/blockqueue.go	82.18% <0.00%> (-2.88%)	⬇️
network/wsPeer.go	74.37% <0.00%> (-1.12%)	⬇️
ledger/acctupdates.go	61.88% <0.00%> (-0.51%)	⬇️
catchup/service.go	70.12% <0.00%> (+0.77%)	⬆️
network/requestTracker.go	71.12% <0.00%> (+0.86%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 916154b...2235976. Read the comment docs.

[algorandskiy]

I don't understand your suggestion though. What would Put(addr, ad, ctype, cidx) do?

Same as PutWithCreatable - based on ctype it checks AssetParams or AppPArams and sets created (if not there) or deleted (if there).

[tolikzinovyev]

This kind of switching seems error-prone. Also what do I do with opting in/out? Another parameter global/local in Put()?

[algorandskiy]

Currently there is no need for local/global (well, it is in my branch and I went with Allocate there).
I think balances interfaces does not provide right abstraction anymore and that's a root of the problem. Originally there were no assets/apps, and no "complex" states like maps of assets/apps. The first modification is in registering asset/app creation/deletion, and the second - state allocation/deallocation. You see, balances interface was originally operating only on AccountData without knowing details about its modifications, but now it also tracks app storage, assets and maps creation that are in fact AccountData modifications but needed to be tracked separately for underlying layers. In my branch it also tracks holdings creation.
It looks like balances (or Put method) needs to be replaced by some high-level ops similar to Move. I attempted to draft it but ended with way to many methods. Just sharing some thoughts.

An alternative to replacing PutWithCreatable can be re-purposing Allocation/Deallocate for creatables as well so all asset params, holdings, app params and local state would follow more-less similar sequence of calls.

[algorandskiy]

Please consider the following alternative: use Allocate/Deallocate for registering events for account data that require additional processing rather than a simple balance record updating

Allocate(addr basics.Address, cidx basics.CreatableIndex, ctype basics.CreatableType, data CreatableOptions) error
Deallocate(addr basics.Address, cidx basics.CreatableIndex, ctype basics.CreatableType, data CreatableOptions) error

basics.CreatableType will need to include AppLocalCreatable and AssetHoldingCreatable to allow using it for holdings and app local states. AssetHoldingCreatable will be no-op now but could be implemented when account data will be using a separate storage for all complex data that is in maps now.

CreatableOptions would be an aggregate of 4 subtypes specific for asset params, holdings (not needed now), app params, and app local states (state schema field).
This looks natural since allocating global state assumes app creation. Allocating AppLocalCreatable would not trigger new app state registering.
Having the same API for both assets and apps would be a good step in a direction of unification of these

[tolikzinovyev]

@algorandskiy, thanks for the feedback! I've made the changes we discussed today.

[algorandskiy]

One small comment remark

[algorandskiy]

This follows the direction of unifying assets and apps (both param and states), so it is a good change.

[tolikzinovyev]

Thanks for review, Pavel! Can somebody merge?

[tsachiherman]

Thanks for review, Pavel! Can somebody merge?

sure. I was waiting to ensure you and Pavel completed your discussion..

[tolikzinovyev]

Thanks!