split Balances Get/Put interface methods into separate methods for creatables #3151

cce · 2021-10-27T12:26:32Z

Summary

This adds extra methods to the apply.Balances interface to offer creatable-specific alternatives to Get and Put, including chasing down all the use cases of Get and Put to call the new interface methods. A new apply.AccountData type restricts access to the full basics.AccountData structure.

In later PRs, the goal is to replace basics.AccountData and migrate to a database that uses separate tables for account data and per-(account, app/asset) data.

Test Plan

All the current tests pass. I'm also running it against mainnet and testnet with CatchpointTracking enabled and comparing the resulting catchpoint hashes with historical catchpoint hash values. I'm also looking at coverage of files in apply to see if there are any new tests I could add to master (and then merge into this branch) to cover any corner cases not currently handled by our existing unit tests.

…eatables

ledger/internal/cow_creatables.go

tolikzinovyev · 2021-10-27T20:33:41Z

Is Balances.Get() going to go away? What is the plan?

tsachiherman · 2021-10-27T21:15:11Z

Is Balances.Get() going to go away? What is the plan?

As you recall from the ledger refactoring documentation, the resources table is going to be separate from the account data. There will definitely be a method that would return the basics of what an account could hold, but all the maps are planned to have a separate accessors.

tolikzinovyev · 2021-10-27T21:32:28Z

So it will return a different type that doesn't contain maps?

algorandskiy

Looks good. Please fix the build, comment new functions, and it is ready to go

ledger/apply/asset.go

algorandskiy · 2021-11-01T09:55:06Z

ledger/apply/asset.go

@@ -406,18 +396,10 @@ func AssetFreeze(cf transactions.AssetFreezeTxnFields, header transactions.Heade
 	}

 	// Get the account to be frozen/unfrozen.
-	record, err := balances.Get(cf.FreezeAccount, false)
+	holding, err := balances.GetAssetHolding(cf.FreezeAccount, cf.FreezeAsset)


wondering why are these "business logic" values passed down to retrieval code?

I'd expect GetAssetHolding to return holdings by address (cf.FreezeAccount in this case), but looks like it also checks/sets cf.FreezeAsset value

cce · 2021-11-01T20:49:34Z

To try and catch anything I missed, I prefixed the maps in AccountData with X and chased down all the users in the ledger package: cce@1b91783#diff-831e406846493c27b22aca1b7482743df94cec8a6957b34059c912ff7996ec28R464

I found AccountData.MinBalance() which counts the size of Assets, AppParams, and AppLocalStates, which is called by logicLedger.MinBalance() and BlockEvaluator.checkMinBalance(). I modified AccountData.MinBalance to accept an external totalAssets, totalAppParams, totalAppLocalStates rather than looking it up from maps.

WDYT — I was thinking of moving AccountData.MinBalance() to an internal helper function in ledger called by both callers, would that be a good idea?

ledger/apply/asset.go

ledger/apply/application.go

…Put(accountdata)

…gicLedger

codecov-commenter · 2021-11-03T01:42:38Z

Codecov Report

Merging #3151 (a094ff5) into feature/unlimited-assets (f72764b) will increase coverage by 0.20%.
The diff coverage is 47.03%.

@@                     Coverage Diff                      @@
##           feature/unlimited-assets    #3151      +/-   ##
============================================================
+ Coverage                     47.23%   47.43%   +0.20%     
============================================================
  Files                           365      370       +5     
  Lines                         58361    59646    +1285     
============================================================
+ Hits                          27564    28296     +732     
- Misses                        27611    28032     +421     
- Partials                       3186     3318     +132

Impacted Files	Coverage Δ
agreement/abstractions.go	`50.00% <ø> (ø)`
agreement/agreementtest/keyManager.go	`100.00% <ø> (ø)`
cmd/goal/clerk.go	`9.18% <0.00%> (ø)`
cmd/goal/multisig.go	`10.00% <0.00%> (ø)`
config/config.go	`47.05% <ø> (ø)`
daemon/algod/api/server/v2/handlers.go	`0.00% <0.00%> (ø)`
daemon/algod/api/server/v2/test/helpers.go	`74.81% <0.00%> (-2.29%)`	⬇️
data/accountManager.go	`0.00% <0.00%> (ø)`
data/transactions/logic/assembler.go	`78.77% <0.00%> (-0.31%)`	⬇️
data/transactions/logic/doc.go	`77.41% <ø> (ø)`
... and 40 more

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 f72764b...a094ff5. Read the comment docs.

jannotti

Had a few questions, but looks like a nice step forward to me.

jannotti · 2021-11-04T17:28:14Z

ledger/apply/application.go

@@ -86,19 +85,22 @@ func createApplication(ac *transactions.ApplicationCallTxnFields, balances Balan
 		return
 	}

+	// look up how many apps they have
+	totalAppParams, err := balances.TotalAppParams(creator)


I don't follow why this became a method, but total TotalExtraAppPages is accessed directly. Aren't they both keeping a cached sum of things we would otherwise have to chase down? I suppose the answer might be, "Yes, so we should move TotalExtraAppPages into nice protected method where we can be sure to track it properly too, but that's not the job of this PR."

I now see that, at least for now, you're not caching a total count. I wonder if we should. The reasoning would be similar, but it does mean writing these objects (man, I really want them to have another name like "AccountHeader" or "AccountBase" so I don't have to distinguish by package name), whenever we add or remove an asset or app. Then again, I guess we already have to, exactly so we can calc min balance.

ledger/apply/apply.go

jannotti · 2021-11-04T17:36:16Z

ledger/apply/apply.go

+// ToApplyAccountData returns apply.AccountData from basics.AccountData
+func ToApplyAccountData(acct basics.AccountData) AccountData {
+	return AccountData{


This makes me start to wonder if apply.AccountData should be basics.AccountBase and be embedded in basics.AccountData. Why does it belong in apply?

It would also make the distinction I mentioned earlier clear. It's not some sort of "view" of the AccountData that the apply package prefers, it's the constant sized base of the full basics.AccountData.

I guess apply.AccountData is a temporary type while we do not have other/better types.
The idea is to have in-mem and persistent data types for components of basics.AccountData

I get that, but it seems to me that this in-memory type is, in fact, the beginning of basics.AccountData, so why not make that explicit?

There was a bigger potential change I considered, where basics.AccountData loses these maps altogether, ledger.Lookup() gets split into separate acct/app/asset methods too, all former users of AccountData are updated to get asset data separately, and the original AccountData type (and its msgp encoding) is moved inside ledger and becomes an implementation detail of account DB, eval, COW, etc.

I didn't think of embedding it as an AccountBase in AccountData because I figured we this PR was moving us towards that goal, starting with apply.Balances.

However looking at what's missing, really only the REST API is left after this I believe.

oh, also I was worried a change to basics.AccountData directly might involving updating a ton of tests.

I see, so the plan might be that basics.AccountData becomes what I was calling basics.AccountBase, and it gets (maybe) embedded in ledger.accountFull. The apply package will take the various pieces it needs, never an entire ledger.accountFull.

y'know, I already changed cowForLogicLedger to have Get() return the new apply.AccountData, which is now out of sync with Ledger.Get() returning basics.AccountData, so I will try plumbing through Ledger.GetAppParams, etc to see how much more changes it would be.

Ok, don't get carried away on my behalf. This is nice enough to do as is, and I don't want it to get so large we we have trouble moving forward incrementally.

I tried tracking down everywhere that was using the maps in basics.AccountData, it was the REST API (account data and dryrun endpoints), tealdbg, and goal, all calling through Ledger.Lookup() basically. I think it's probably best for now to have ledger.Ledger just hide the logic behind Lookup() of joining the asset/app data with account data, rather than change all the tests and callers to Lookup that use basics.AccountData.

That's in: cce/go-algorand@split-balances...split-balances-hide-accountdata-maps

jannotti · 2021-11-04T17:37:26Z

ledger/apply/apply.go

+// AssignAccountData assigns the contents of apply.AccountData to the fields in basics.AccountData
+func AssignAccountData(a *basics.AccountData, acct AccountData) {
+	a.Status = acct.Status
+	a.MicroAlgos = acct.MicroAlgos


Now even more so. This would become a simple assignment.

ledger/apply/apply.go

jannotti · 2021-11-04T17:44:01Z

ledger/apply/asset.go

 			Amount: cc.AssetParams.Total,
 		}

-		if len(record.Assets) > balances.ConsensusParams().MaxAssetsPerAccount {
-			return fmt.Errorf("too many assets in account: %d > %d", len(record.Assets), balances.ConsensusParams().MaxAssetsPerAccount)
+		if totalAssets >= balances.ConsensusParams().MaxAssetsPerAccount {


nit: check this before the above work?

yeah, I was overoptimizing for min diff length, I think there are a few places like this I can pull the "too many X" checks up

No strong need, sometimes I don't really notice what's new and what was already there.

ledger/internal/cow_creatables.go

…cloneAssetParams to test code

* Add error strings to v2 TEAL compile endpoint * Move number of errors to end so it is consistent with goal clerk compile * Refactor ReportProblems() to take io.Writer * Don't report number of errors on the API call

* add abi encoding * accord with go-algorand code format * minor modification * add partition test * resolve review, need more testcase rewrite * move from cmd/goal to data, resolve review * rewrite is-dynamic, dynamic array * update dynamic array 0 element support, test needed * minor * minor * minor * add more testcase for abi encode/decode * update comments in encoding test * minor * attempt to split abi.go into 2 files * separate abi files to smaller files * resolve reviews, work on random gen tuple value encode/decode * add random tuple test * remove math-rand, use crypto-rand * minor * minor * some change requested from community * fix for 1 corner case * resolve review comments * resolve review comments * minor * minor * update encode slot capacity * minor * resolve reviews * minor update on bool bytelen calculate * update encode/decode from types * random test remain to be modified * testing variable renaming, encode int support (u)int types * update test scripts and remove value struct * follow golint * partly resolving comments * whoops uint encoding update * update int decode to primitive types method * go fmt * update parseAppArg to accept abi input (attempt) * need to check cmdline arg validity * update unmarshal from JSON in ABI type * unmarshal from json for ABI type * update ABI type unmarshal values from JSON bytes * update ABI methods for string/array/address * update unmarshal from JSON in abi * fix for error in ufixed json unmarshal * fix * update on method sub command * minor * probably better separate abi json to a single file * i just want to add a required flag plz... * minor fix on interface from json * consider some rough test cases * minor * add partition test * update static uint test * update marshal/unmarshal json methods for abi * marshal byte array to b64 string * abi json polish * update golangci lint rules * revert golangci config * update method impl * update method signature return type check * minor * copy-paste code from call app cmd * minor * add method flag to txn flags * minor * update changes * minor * moving helper functions to abi * update comments * update method app call * resolve part in abi impl * add oncomplete support * minor * try to use stringarrayvar * minor * update goal return log handing process * go simple * add a line of e2e test for now * update * minor * minor * minor * go fmt * approval/clear prog nil * discard all changes to e2d-app-cross-round, going to write separately e2e test * update e2d tests * check ret valu * use constant * resolve review partly * resolve review on code reformatting * resolve review on code reformatting, use code chunk for datadir and client * go fmt * export tuple type maker * update comments in e2e test * update filter empty string * resolve issues with JSON abi * minor

* adding missing to opcode docs for external gets * update TEAL_opcodes.md

## Summary * new `ParticipationRegistry` interface designed to manage keys and collect usage metrics. * new REST endpoints for interacting with keys. * improved `goal account partkeyinfo` and `goal account listpartkeys` formatting. ## Test Plan New unit and integration tests.

Integration tests (specifically, e2e-go tests) seem to be having resource issues lately, so this returns integration tests to using large instances (4 cores, ~16GB RAM) after they were downsized to medium (2 cores ~8GB RAM) in algorand#3095. Nightly integration tests are already using large VMs.

## Summary When implementing the catchpoint tracker, I missed the re-initilization location for some of the local variables. This would generate incorrect catchpoint labels after a node completes a fast-catchup. algorand#3085 ## Test Plan - [x] Add unit tests - [x] Perform manual testing

split Balances Get/Put interface methods into separate methods for cr…

28752a5

…eatables

tsachiherman reviewed Oct 27, 2021

View reviewed changes

ledger/internal/cow_creatables.go Outdated Show resolved Hide resolved

clone maps in balances.PutX methods

1c9f52d

algorandskiy reviewed Nov 1, 2021

View reviewed changes

cce added 2 commits November 1, 2021 15:12

consolidate to ErrCreatableNotFound

deb7d42

add missing user of ErrCreatableNotFound

c4f64e1

cce commented Nov 2, 2021

View reviewed changes

ledger/apply/asset.go Show resolved Hide resolved

cce commented Nov 2, 2021

View reviewed changes

ledger/apply/application.go Outdated Show resolved Hide resolved

cce added 4 commits November 2, 2021 12:38

update GetX to return (param, bool, error)

5026682

clone for delete methods

eea0831

fix issue where DeleteX(resource) or PutX(resource) is called before …

6db6416

…Put(accountdata)

update testBalances, testBalancesPass, mockBalances, and mockCowForLo…

f9f007e

…gicLedger

cce added 4 commits November 3, 2021 09:11

Balances: use apply.AccountData instead of basics.AccountData

ddacdc6

add balances.CloseAccount() and fix tests

8eb8baa

rearrange code Put and DeleteX calls to be like before 6db6416

175d53d

fix tealdbg tests calling Balances methods

0f9ac78

cce requested a review from jannotti November 4, 2021 17:10

jannotti reviewed Nov 4, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into split-balances

5043409

cce marked this pull request as ready for review November 5, 2021 15:29

cce and others added 5 commits November 5, 2021 11:32

fix reviewdog errors

6b9ae4b

rename TotalX => CountX, CheckX => HasX

ff762f0

address some code review comments

471ac04

move unused cloneAppParams, cloneAppLocalStates, cloneAssetHoldings, …

588373f

…cloneAssetParams to test code

cce changed the title ~~WIP: split Balances Get/Put interface methods into separate methods for creatables~~ Split Balances Get/Put interface methods into separate methods for creatables Nov 8, 2021

cce changed the title ~~Split Balances Get/Put interface methods into separate methods for creatables~~ split Balances Get/Put interface methods into separate methods for creatables Nov 8, 2021

ahangsu and others added 5 commits November 9, 2021 18:07

Clarify get_ex opcode docs (algorand#3208)

3937401

* adding missing to opcode docs for external gets * update TEAL_opcodes.md

cce changed the base branch from master to feature/unlimited-assets November 16, 2021 15:13

Merge remote-tracking branch 'origin/master' into split-balances

a094ff5

algorandskiy approved these changes Nov 18, 2021

View reviewed changes

cce merged commit c737cb9 into algorand:feature/unlimited-assets Nov 18, 2021

cce deleted the split-balances branch November 18, 2021 20:59

tsachizehub added the Team Carbon-11 label Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split Balances Get/Put interface methods into separate methods for creatables #3151

split Balances Get/Put interface methods into separate methods for creatables #3151

cce commented Oct 27, 2021 •

edited

Loading

tolikzinovyev commented Oct 27, 2021

tsachiherman commented Oct 27, 2021

tolikzinovyev commented Oct 27, 2021

algorandskiy left a comment

algorandskiy Nov 1, 2021

cce Nov 2, 2021

algorandskiy Nov 3, 2021

cce commented Nov 1, 2021 •

edited

Loading

codecov-commenter commented Nov 3, 2021 •

edited

Loading

jannotti left a comment

jannotti Nov 4, 2021 •

edited

Loading

jannotti Nov 4, 2021

jannotti Nov 4, 2021

algorandskiy Nov 4, 2021

jannotti Nov 4, 2021

cce Nov 5, 2021

cce Nov 5, 2021

jannotti Nov 5, 2021

cce Nov 5, 2021

jannotti Nov 5, 2021

cce Nov 5, 2021

cce Nov 5, 2021

jannotti Nov 4, 2021

jannotti Nov 4, 2021

cce Nov 5, 2021

jannotti Nov 5, 2021

split Balances Get/Put interface methods into separate methods for creatables #3151

split Balances Get/Put interface methods into separate methods for creatables #3151

Conversation

cce commented Oct 27, 2021 • edited Loading

Summary

Test Plan

tolikzinovyev commented Oct 27, 2021

tsachiherman commented Oct 27, 2021

tolikzinovyev commented Oct 27, 2021

algorandskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cce commented Nov 1, 2021 • edited Loading

codecov-commenter commented Nov 3, 2021 • edited Loading

Codecov Report

jannotti left a comment

Choose a reason for hiding this comment

jannotti Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cce commented Oct 27, 2021 •

edited

Loading

cce commented Nov 1, 2021 •

edited

Loading

codecov-commenter commented Nov 3, 2021 •

edited

Loading

jannotti Nov 4, 2021 •

edited

Loading