REST API: add support for optional "round" parameter to lookup queries #4018
Conversation
| // getOptionalRound converts an optional round argument into a basics.Round(0) for "latest" | ||
| // or a non-negative integer. | ||
| // TODO: could change this to parse a string argument and allow param to use string value "latest" | ||
| func getOptionalRound(round *uint64) (basics.Round, error) { |
There was a problem hiding this comment.
This interface seems like a funny middle ground. If we only care about supporting "latest" or a number, I think it's fine to only take numbers, and remove all of the ceremony around an error that never happens. If we wanted to support of kind of tokens besides "latest" we would need to change the input type and the output type (because using 0 as a special case would be insufficient). I lean toward guessing there are no other good named rounds, so I'd rather just skip the error stuff for now.
| // totalsImpl returns the online totals of all accounts at the end of round rnd. | ||
| func (au *accountUpdates) totalsImpl(rnd basics.Round) (ledgercore.AccountTotals, error) { |
There was a problem hiding this comment.
Perhaps we do this a lot, but "Impl" always bugs be as a name because it doesn't say anything about why it's pulled out. Here I guess the point is that it is totalsWithoutLocking or some such. (I'd love a more concise convention for such cases.)
Here, since it's only called from Totals, I guess I'd rather just inline it.
| data, rnd, withoutRewards, err := l.accts.lookupLatest(0, addr) | ||
| if err != nil { | ||
| return basics.AccountData{}, basics.Round(0), basics.MicroAlgos{}, err | ||
| } | ||
|
|
||
| return data, rnd, withoutRewards, nil |
There was a problem hiding this comment.
| data, rnd, withoutRewards, err := l.accts.lookupLatest(0, addr) | |
| if err != nil { | |
| return basics.AccountData{}, basics.Round(0), basics.MicroAlgos{}, err | |
| } | |
| return data, rnd, withoutRewards, nil | |
| return l.accts.lookupLatest(0, addr) |
| defer l.trackerMu.RUnlock() | ||
|
|
||
| // Intentionally apply (pending) rewards up to rnd. | ||
| data, rnd, withoutRewards, err := l.accts.lookupLatest(round, addr) |
There was a problem hiding this comment.
same suggestion, but github won't let me do it inline because this isn't all new code
|
@tzaffi would we need to change the algod/indexer API parity checker to allow this? |
Looks like this is going to pass the indexer parity check as is (I tested this locally). This may be because the latest iteration is only looking at the |
| @@ -1084,7 +1129,7 @@ func (v2 *Handlers) GetPendingTransactions(ctx echo.Context, params generated.Ge | |||
|
|
|||
| // GetApplicationByID returns application information by app idx. | |||
| // (GET /v2/applications/{application-id}) | |||
| func (v2 *Handlers) GetApplicationByID(ctx echo.Context, applicationID uint64) error { | |||
| func (v2 *Handlers) GetApplicationByID(ctx echo.Context, applicationID uint64, params generated.GetApplicationByIDParams) error { | |||
| appIdx := basics.AppIndex(applicationID) | |||
| ledger := v2.Node.LedgerForAPI() | |||
| creator, ok, err := ledger.GetCreator(basics.CreatableIndex(appIdx), basics.AppCreatable) | |||
There was a problem hiding this comment.
Just realized this does not use GetCreatorForRound
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4018 +/- ##
==========================================
+ Coverage 49.77% 54.36% +4.59%
==========================================
Files 409 390 -19
Lines 69157 48559 -20598
==========================================
- Hits 34420 26400 -8020
+ Misses 31014 19930 -11084
+ Partials 3723 2229 -1494 ☔ View full report in Codecov by Sentry. |
Summary
This demonstrates how it would be possible to add support for providing a round number to ledger lookup methods accessed by the REST API.
Test Plan
Existing tests should pass, extra tests could be added for querying data using round numbers that are too old to be retrieved.