ledger: fix lookupLatest usage of the resources cache #3939
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* In some situations the method might incorrectly count deleted items towards total items * This might result not returning all the resources to REST API clients
Codecov Report
@@ Coverage Diff @@
## master #3939 +/- ##
==========================================
+ Coverage 49.79% 49.80% +0.01%
==========================================
Files 409 409
Lines 68892 68897 +5
==========================================
+ Hits 34303 34313 +10
+ Misses 30884 30881 -3
+ Partials 3705 3703 -2
Continue to review full report at Codecov.
|
cce
reviewed
Apr 30, 2022
| @@ -42,29 +42,34 @@ type AppResource struct { | |||
|
|
|||
| // AssignAccountResourceToAccountData assignes the Asset/App params/holdings contained | |||
| // in the AccountResource to the given basics.AccountData, creating maps if necessary. | |||
| func AssignAccountResourceToAccountData(cindex basics.CreatableIndex, resource AccountResource, ad *basics.AccountData) { | |||
| func AssignAccountResourceToAccountData(cindex basics.CreatableIndex, resource AccountResource, ad *basics.AccountData) (assigned bool) { | |||
There was a problem hiding this comment.
maybe a comment like
Suggested change
| func AssignAccountResourceToAccountData(cindex basics.CreatableIndex, resource AccountResource, ad *basics.AccountData) (assigned bool) { | |
| // Returns true if the AccountResource contained a new or updated resource, and false if the AccountResource contained no changes (indicating the resource was deleted). | |
| func AssignAccountResourceToAccountData(cindex basics.CreatableIndex, resource AccountResource, ad *basics.AccountData) (assigned bool) { |
There was a problem hiding this comment.
I guess another way to do it (rather than mix it into the Assign method) would be to have a method like
// IsEmpty is maybe not a great name, perhaps there is something clearer
func (r *AccountResource) IsEmpty() bool {
return r.AssetParams == nil && r.AssetHolding == nil && r.AppLocalState == nil && r.AppParams == nil
}There was a problem hiding this comment.
I thought about checking in addResource but I think AssignAccountResourceToAccountData is a right place because 1) it has "insertion" logic 2) it basically creates the output data structure 3) it is used only in addResource.
Updated the AssignAccountResourceToAccountData comment as suggested.
cce
previously approved these changes
May 1, 2022
cce
approved these changes
May 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
deleted items towards total items
REST API clients
Test Plan
Test added