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: Txn Group Delta Apis #5350

Merged
merged 15 commits into from
May 9, 2023
Merged

Conversation

Eric-Warehime
Copy link
Contributor

Summary

Implements two new APIs:

  • /v2/deltas/{round}/txn/group
  • /v2/deltas/txn/group/{id}

These expose the state delta objects for groups of transactions.

Test Plan

Adding handler tests as the PR progresses.

@Eric-Warehime Eric-Warehime marked this pull request as ready for review May 2, 2023 16:52
@@ -91,6 +92,7 @@ type LedgerForAPI interface {
Block(rnd basics.Round) (blk bookkeeping.Block, err error)
AddressTxns(id basics.Address, r basics.Round) ([]transactions.SignedTxnWithAD, error)
GetStateDeltaForRound(rnd basics.Round) (ledgercore.StateDelta, error)
GetTracer() logic.EvalTracer
Copy link
Contributor

@winder winder May 3, 2023

Choose a reason for hiding this comment

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

nit: Should this return an interface that gets cast to the EvalTracer?

Nevermind, this appears to be the interface

Comment on lines 1719 to 1722
tracer, ok := v2.Node.LedgerForAPI().GetTracer().(*eval.TxnGroupDeltaTracer)
if !ok {
return serviceUnavailable(ctx, err, errFailedRetrievingTracer, v2.Log)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this handle a nil tracer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've added a test for this case as well just to be sure.

I'm not sure that 503 is the exactly the correct http response though--open to suggestions on better responses there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think 503 is a good choice, isn't it supposed to be fore temporary conditions? I'd probably go with 501 or 500.

@@ -72,29 +102,34 @@ func (tracer *TxnGroupDeltaTracer) AfterTxnGroup(ep *logic.EvalParams, deltas *l
}

// GetDeltasForRound supplies all StateDelta objects for txn groups in a given rnd
func (tracer *TxnGroupDeltaTracer) GetDeltasForRound(rnd basics.Round) ([]ledgercore.StateDelta, error) {
func (tracer *TxnGroupDeltaTracer) GetDeltasForRound(rnd basics.Round) ([]TxnGroupDeltaWithIds, error) {
rndEntries, exists := tracer.txnGroupDeltas[rnd]
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like txnGroupDeltas needs a RW lock.

Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

I created a child PR while going through this PR to add some additional handler checks for my own sanity: Eric-Warehime#7

daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
algochoi
algochoi previously approved these changes May 3, 2023
Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

Just need to regenerate the specs if we change the 503 to 501?

daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved
algochoi
algochoi previously approved these changes May 5, 2023
Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

LGTM, I am not particularly opinionated about what error should be returned for a nil tracer.

@@ -22,6 +22,7 @@ import (
"encoding/base64"
"errors"
"fmt"
"github.com/algorand/go-algorand/ledger/eval"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: import groups

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Just one small change needed in the OpenAPI, otherwise LGTM.

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #5350 (6cb9322) into master (9e980d3) will increase coverage by 0.02%.
The diff coverage is 44.61%.

@@            Coverage Diff             @@
##           master    #5350      +/-   ##
==========================================
+ Coverage   55.28%   55.30%   +0.02%     
==========================================
  Files         454      454              
  Lines       63772    63827      +55     
==========================================
+ Hits        35254    35300      +46     
- Misses      26115    26129      +14     
+ Partials     2403     2398       -5     
Impacted Files Coverage Δ
daemon/algod/api/server/v2/handlers.go 0.83% <0.00%> (-0.03%) ⬇️
daemon/algod/api/server/v2/utils.go 11.84% <0.00%> (-0.11%) ⬇️
ledger/ledger.go 71.03% <0.00%> (-0.36%) ⬇️
ledger/eval/txntracer.go 94.23% <96.66%> (+3.60%) ⬆️

... and 14 files with indirect coverage changes

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

@winder winder merged commit 2bb2516 into algorand:master May 9, 2023
9 checks passed
@winder winder changed the title algod: Txn Group Delta Apis api: Txn Group Delta Apis May 9, 2023
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.

None yet

3 participants