Algod : merge unlimited assets to master #3652
Conversation
…cloneAssetParams to test code
…ration (#3195) * Initial support for ExtendApplicationStorage * minor bugfix. * better documentation * update testing protocol. * fix consensus protocol json * rename ExtendApplicationStorage -> EnableExplicitAccountResourceTracking * rename EnableExplicitAccountResourceTracking->EnableAccountDataResourceSeparation
## Summary This PR adds the migration to the new schema designed at https://docs.google.com/document/d/1E9sQvWfu9uOYvai4VjWIyUDY9Mo8bhnBX92qXh1y54o/edit#, by adding a new database upgrade. The encoded AccountData is being re-encoded into the new schema, and the old data is dropped. It also include the long-waiting PR : #1974 The migration implemented in this PR is currently hard-gated. We should remove that gating only once the rest of the logic would be capable of working with the new schema. ## Test Plan Unit tests updated/added.
split Balances Get/Put interface methods into separate methods for creatables
…orand into feature/unlimited-assets
…nlineAccountBalance (#3241) ## Summary extract the logic of NormalizedOnlineBalance into NormalizedOnlineAccountBalance. The change is quite benign, but would allow future PRs to complete the calculation of the normalized balance without having an AccountData object. ## Test Plan Use existing unit tests.
## Summary Part 2 of `apply.balances` and `cow` changes. This PR introduces partial deltas (`NewAccountDelta`) and some glue to make it work with existing old schema. Most of this glue code must be removed when a new schema is enabled. ## Test Plan - [x] Existing tests - [x] Add `Total*` fields validation test
## Summary MergeInMatchingAccounts requires addresses per resource but storing addresses there makes the code much slower. Lets see if MergeInMatchingAccounts can be eliminated altogether on the schema switch. ## Test Plan Added new test for triggering missed base account data in StateDelta for some application resources modification.
## Summary This is follow up PR for #3292 ## Test Plan This is mostly existing tests modification
…orand into feature/unlimited-assets
## Summary Create consensus upgrade to V32 ## Test Plan Use existing unit tests.
… feature/unlimited-assets-api-split-accountresource
…3666) ## Summary This PR replaces the existing account preloading during validating with a more transaction-type oriented prefetcher. The prefetcher examine the transaction type, and preload all the required resources ( account data, assets and applications ). ## Test Plan Unit test added. ## Benchmarks The following (existing) benchmarks were executed, and no performance difference was noted : * BenchmarkBlockEvaluatorDiskFullAppOptIns * BenchmarkBlockEvaluatorDiskAppOptIns * BenchmarkBlockEvaluatorDiskNoCrypto * BenchmarkBlockEvaluatorDiskCrypto * BenchmarkBlockEvaluatorRAMNoCrypto * BenchmarkBlockEvaluatorRAMCrypto The benchmark `BenchmarkBlockEvaluatorDiskAppCalls`, which was designed for applications, shown a notable improvements, dropping the transaction processing time from 42902ns to 33465ns.
…accountresource REST API: split msgpack API type AccountResourceModel into two
| @@ -1600,10 +1784,6 @@ | |||
| "type": "integer", | |||
| "x-go-name": "AssetID" | |||
| }, | |||
| "creator": { | |||
| "description": "Address that created this asset. This is the address where the parameters for this asset can be found, and also the address where unwanted asset units can be sent in the worst case.", | |||
There was a problem hiding this comment.
Will this impact sdk backwards compatibility? Please comment.
There was a problem hiding this comment.
As is, yes it will. I'd like to suggest we manually retain this field in the SDK models so they don't have an explicitly breaking change. The field can just be empty if algod doesn't return it.
There was a problem hiding this comment.
IMO it would be preferable to introduce a breaking change than provide an empty value. That way you would find out that you need to pull the creator from /v2/assets/{asset-id} instead. However I don't have a lot of experience with how we deprecate functionality in the SDKs or warn users of issues like this, whether a compiler error or compiler warning is more typical.
There was a problem hiding this comment.
Related: algorand/indexer#900 (comment)
|
|
||
| // IsEmpty return true if any of the fields other then the UpdateRound are non-zero. | ||
| func (ba *baseAccountData) IsEmpty() bool { | ||
| return ba.Status == 0 && |
There was a problem hiding this comment.
I understand that because of the round fields, we can't return *ba == baseAccountData{} do you think it is worth having an inner type for it?
There was a problem hiding this comment.
I think it's a bit of an overkill to have an embedded datatype if you're going to use it in exactly one location.
There is a unit test that verify that nothing is being missed here : TestBaseAccountDataIsEmpty.
There was a problem hiding this comment.
it could also be used for the set and the get.
but as long as we don't "miss" zeroing or setting fields. it is good
## Summary AccountApplicationInformation was calling `ledger.LookupResource` with `basics.AssetCreatable` instead of `basics.AppCreatable`, resulting in a consistent failure. ## Test Plan Unit test will follow on a subsequent PR.
…3708) ## Summary This replaces the various Ledger interfaces' `LookupResource` method with two more specific `LookupApplication` and `LookupAsset` methods. Following up on code review feedback from #3652. ## Test Plan Existing tests should pass, including the ones that implement their own mock ledger.
Summary
This PR adds support of for unlimited assets.
Done:
Test Plan
Extend, update and add new unit tests.