Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ledger: Make AccountDelta fields visible for serialization. #4620

Merged
merged 4 commits into from
Oct 24, 2022

Conversation

winder
Copy link
Contributor

@winder winder commented Oct 1, 2022

Summary

The StateDelta object is part of Indexer's interface and I'd like to serialize it to a file, but some of the fields are private.

@winder winder self-assigned this Oct 1, 2022
@winder winder changed the title Make AccountDelta fields visible for serialization. ledger: Make AccountDelta fields visible for serialization. Oct 1, 2022
@codecov
Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Merging #4620 (6327e3e) into master (36fc7f9) will increase coverage by 0.03%.
The diff coverage is 19.60%.

@@            Coverage Diff             @@
##           master    #4620      +/-   ##
==========================================
+ Coverage   54.37%   54.40%   +0.03%     
==========================================
  Files         407      407              
  Lines       52389    52389              
==========================================
+ Hits        28486    28503      +17     
+ Misses      21520    21507      -13     
+ Partials     2383     2379       -4     
Impacted Files Coverage Δ
ledger/ledgercore/statedelta.go 7.32% <19.60%> (ø)
network/wsNetwork.go 65.34% <0.00%> (-0.19%) ⬇️
catchup/service.go 68.64% <0.00%> (+0.49%) ⬆️
ledger/testing/randomAccounts.go 56.83% <0.00%> (+0.62%) ⬆️
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
network/wsPeer.go 68.44% <0.00%> (+1.94%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@algorandskiy
Copy link
Contributor

I suggest not to expose this data type. Or at least not entirely (keep *Cache) portion private and reconstruct after deserialization as needed - they are just lookup helpers, and not sure if they needed by the Indexer at all.

@winder
Copy link
Contributor Author

winder commented Oct 3, 2022

I suggest not to expose this data type. Or at least not entirely (keep *Cache) portion private and reconstruct after deserialization as needed - they are just lookup helpers, and not sure if they needed by the Indexer at all.

This PR was mostly for discussion. I have a branch on Indexer which is writing this to a file and loading Indexer with these files instead of a live algod. It seems to work fine and might be a simpler alternative to a new API.

In either case the data needs to be serialized somehow.

@algorandskiy
Copy link
Contributor

How's the StateDelta is used (which methods are needed) ? I guess only GetByIdx, Len and GetResource are needed. Only the last one requires maps for lookup.

@@ -160,19 +157,19 @@ type AssetResourceRecord struct {
// The map would point the address/address+creatable id onto the index of the
// element within the slice.
type AccountDeltas struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@algorandskiy converting to a thread

How's the StateDelta is used (which methods are needed) ? I guess only GetByIdx, Len and GetResource are needed. Only the last one requires maps for lookup.

When the object has been created by the ledger, we only need the public methods. This is all code that Tolic and Tsachi wrote. The entry point is here: https://github.com/algorand/indexer/blob/develop/idb/postgres/internal/writer/writer.go#L327

It's split out pretty nicely in writeAccountDeltas:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so only accts, appResources, assetResources should be exported for serialization. The downside is StateDelta would miss all maps after protocol.Decode call.

algorandskiy
algorandskiy previously approved these changes Oct 17, 2022
ledger/ledgercore/statedelta.go Outdated Show resolved Hide resolved
Comment on lines 694 to +700
func (ad *AccountDeltas) GetAllAppResources() []AppResourceRecord {
return ad.appResources
return ad.AppResources
}

// GetAllAssetResources returns all AssetResourceRecords
func (ad *AccountDeltas) GetAllAssetResources() []AssetResourceRecord {
return ad.assetResources
return ad.AssetResources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we can just add

func (ad *AccountDeltas) GetAllAccounts() []NewBalanceRecord {
    return ad.accts
}

so that we have accessors for all the private fields.

@winder winder merged commit fbd5b17 into algorand:master Oct 24, 2022
@winder winder deleted the will/public-account-deltas branch October 24, 2022 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants