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

API: Experimental full assets for account endpoint. #5948

Merged
merged 18 commits into from
May 3, 2024

Conversation

gmalouf
Copy link
Contributor

@gmalouf gmalouf commented Mar 6, 2024

Summary

Support includes always grabbing corresponding asset parameters, supporting pagination via the next / limit parameters, and enforcing max batch caps. Support for this endpoint has also been added to goal, keeping in mind this is experimental for this initial release.

Closes #5250

Test Plan

Initial smoke tests against a caught up testnet node showed the pagination and limit queries working well. Added functional tests for the handler and query layers respectively.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 10.64516% with 277 lines in your changes are missing coverage. Please review.

Project coverage is 55.97%. Comparing base (ff2c966) to head (737c178).

Files Patch % Lines
ledger/store/trackerdb/sqlitedriver/sql.go 21.50% 72 Missing and 1 partial ⚠️
ledger/store/trackerdb/sqlitedriver/schema.go 10.52% 47 Missing and 4 partials ⚠️
daemon/algod/api/server/v2/handlers.go 0.00% 47 Missing ⚠️
cmd/goal/account.go 10.20% 44 Missing ⚠️
ledger/acctupdates.go 0.00% 32 Missing ⚠️
ledger/store/trackerdb/sqlitedriver/trackerdbV2.go 0.00% 10 Missing ⚠️
ledger/store/trackerdb/sqlitedriver/testing.go 18.18% 9 Missing ⚠️
libgoal/libgoal.go 0.00% 5 Missing ⚠️
ledger/ledger.go 0.00% 3 Missing ⚠️
...edger/store/trackerdb/generickv/accounts_reader.go 0.00% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5948      +/-   ##
==========================================
- Coverage   56.18%   55.97%   -0.21%     
==========================================
  Files         482      482              
  Lines       67989    68287     +298     
==========================================
+ Hits        38199    38225      +26     
- Misses      27201    27462     +261     
- Partials     2589     2600      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

More to look at, but want to get my big comment submitted.

daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
ledger/acctupdates.go Outdated Show resolved Hide resolved
@SilentRhetoric
Copy link

Thank you!!

daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Show resolved Hide resolved
ledger/acctupdates.go Outdated Show resolved Hide resolved
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
@gmalouf gmalouf force-pushed the asset-fullparams-batch-endpoint branch from 4a3c512 to 38201ed Compare April 1, 2024 21:53
@gmalouf gmalouf marked this pull request as ready for review April 4, 2024 20:46
daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved
ledger/store/trackerdb/sqlitedriver/sql.go Outdated Show resolved Hide resolved
ledger/store/trackerdb/testsuite/accounts_kv_test.go Outdated Show resolved Hide resolved
ledger/store/trackerdb/sqlitedriver/sql.go Show resolved Hide resolved
cmd/goal/account.go Show resolved Hide resolved
@gmalouf gmalouf force-pushed the asset-fullparams-batch-endpoint branch from caef69c to bbbd3c6 Compare April 30, 2024 01:14
@gmalouf gmalouf requested review from cce and jasonpaulos April 30, 2024 14:48
…t includes always grabbing corresponding asset parameters, supporting pagination via the next / limit parameters, and enforcing max batch caps.
…query one item extra each time to see if we have actually tokenized through all results before returning. Handler now deals with asset params being optional (in the case of a deleted asset).
…against asset creator information iff it is available. Updated result parsing logic to handle this data conditionally being present. Added new functional test exercising the limit query itself, including several failure modes. Lastly, tweak handler to check if creator address is set to its zero value in determining whether to return result with asset params set.
…aging a left join with assetcreators to populate much of the ctypes on updated resources table.
…aving to create another table and full copy.
…tchpoint and setting the db version properly (to ensure columns present during prepared statement init).
@gmalouf gmalouf force-pushed the asset-fullparams-batch-endpoint branch from eb6864a to 963027d Compare April 30, 2024 17:36
ledger/acctupdates.go Outdated Show resolved Hide resolved
ledger/acctupdates.go Outdated Show resolved Hide resolved
ledger/catchpointfilewriter_test.go Show resolved Hide resolved
ledger/store/trackerdb/sqlitedriver/schema.go Show resolved Hide resolved
ledger/store/trackerdb/sqlitedriver/sql.go Outdated Show resolved Hide resolved
ledger/store/trackerdb/sqlitedriver/sql.go Show resolved Hide resolved
ledger/acctupdates.go Outdated Show resolved Hide resolved
algorandskiy
algorandskiy previously approved these changes May 2, 2024
ledger/acctupdates.go Show resolved Hide resolved
ledger/store/trackerdb/sqlitedriver/schema.go Show resolved Hide resolved
ledger/store/trackerdb/sqlitedriver/schema.go Show resolved Hide resolved
Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Everything looks good, except I'd like to see more robust checking of the goal command. I left a suggestion which adds this. Willing to approve once that's in.

test/scripts/e2e_subs/goal-account-asset.sh Outdated Show resolved Hide resolved
@gmalouf gmalouf merged commit fe78417 into algorand:master May 3, 2024
18 checks passed
@gmalouf gmalouf deleted the asset-fullparams-batch-endpoint branch May 3, 2024 19:24
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.

Provide full asset information on /v2/accounts/{address}
6 participants