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

Support search by zero address. #771

Merged
merged 6 commits into from
Nov 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions accounting/accounting.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package accounting
import (
"github.com/algorand/go-algorand/data/basics"
"github.com/algorand/go-algorand/data/transactions"
"github.com/algorand/go-algorand/protocol"
)

// GetTransactionParticipants calls function `add` for every address referenced in the
Expand All @@ -11,15 +12,31 @@ func GetTransactionParticipants(stxnad *transactions.SignedTxnWithAD, includeInn
txn := &stxnad.Txn

add(txn.Sender)
add(txn.Receiver)
add(txn.CloseRemainderTo)
add(txn.AssetSender)
add(txn.AssetReceiver)
add(txn.AssetCloseTo)
add(txn.FreezeAccount)

for _, address := range txn.ApplicationCallTxnFields.Accounts {
add(address)
switch txn.Type {
case protocol.PaymentTx:
add(txn.Receiver)
// Close address is optional.
if !txn.CloseRemainderTo.IsZero() {
add(txn.CloseRemainderTo)
}
case protocol.AssetTransferTx:
// If asset sender is non-zero, it is a clawback transaction. Otherwise,
// the transaction sender address is used.
if !txn.AssetSender.IsZero() {
add(txn.AssetSender)
}
add(txn.AssetReceiver)
// Asset close address is optional.
if !txn.AssetCloseTo.IsZero() {
add(txn.AssetCloseTo)
}
case protocol.AssetFreezeTx:
add(txn.FreezeAccount)
case protocol.ApplicationCallTx:
for _, address := range txn.ApplicationCallTxnFields.Accounts {
add(address)
}
}

if includeInner {
Expand Down
10 changes: 0 additions & 10 deletions api/converter_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -556,16 +556,6 @@ func assetParamsToAssetQuery(params generated.SearchForAssetsParams) (idb.Assets
func transactionParamsToTransactionFilter(params generated.SearchForTransactionsParams) (filter idb.TransactionFilter, err error) {
var errorArr = make([]string, 0)

// Round + min/max round
if params.Round != nil && (params.MaxRound != nil || params.MinRound != nil) {
errorArr = append(errorArr, errInvalidRoundAndMinMax)
}

// If min/max are mixed up
if params.Round == nil && params.MinRound != nil && params.MaxRound != nil && *params.MinRound > *params.MaxRound {
errorArr = append(errorArr, errInvalidRoundMinMax)
}

// Integer
filter.MaxRound = uintOrDefault(params.MaxRound)
filter.MinRound = uintOrDefault(params.MinRound)
Expand Down
45 changes: 24 additions & 21 deletions api/error_messages.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,30 @@ import (
)

const (
errInvalidRoundAndMinMax = "cannot specify round and min-round/max-round"
errInvalidRoundMinMax = "min-round must be less than max-round"
errUnableToParseAddress = "unable to parse address"
errInvalidCreatorAddress = "found an invalid creator address"
errUnableToParseBase64 = "unable to parse base64 data"
errUnableToParseDigest = "unable to parse base32 digest data"
errUnableToParseNext = "unable to parse next token"
errUnableToDecodeTransaction = "unable to decode transaction bytes"
errFailedSearchingAccount = "failed while searching for account"
errNoAccountsFound = "no accounts found for address"
errNoAssetsFound = "no assets found for asset-id"
errNoTransactionFound = "no transaction found for transaction id"
errMultipleTransactions = "multiple transactions found for this txid, please contact us this shouldn't happen"
errMultipleAccounts = "multiple accounts found for this address, please contact us this shouldn't happen"
errMultipleAssets = "multiple assets found for this id, please contact us this shouldn't happen"
errMultiAcctRewind = "multiple accounts rewind is not supported by this server"
errRewindingAccount = "error while rewinding account"
errLookingUpBlock = "error while looking up block for round"
errTransactionSearch = "error while searching for transaction"
errSpecialAccounts = "indexer doesn't support fee sink and rewards pool accounts, please refer to algod for relevant information"
errFailedLoadSpecialAccounts = "failed to retrieve special accounts"
errInvalidRoundAndMinMax = "cannot specify round and min-round/max-round"
errInvalidRoundMinMax = "min-round must be less than max-round"
errUnableToParseAddress = "unable to parse address"
errInvalidCreatorAddress = "found an invalid creator address"
errUnableToParseBase64 = "unable to parse base64 data"
errUnableToParseDigest = "unable to parse base32 digest data"
errUnableToParseNext = "unable to parse next token"
errUnableToDecodeTransaction = "unable to decode transaction bytes"
errFailedSearchingAccount = "failed while searching for account"
errNoAccountsFound = "no accounts found for address"
errNoAssetsFound = "no assets found for asset-id"
errNoTransactionFound = "no transaction found for transaction id"
errMultipleTransactions = "multiple transactions found for this txid, please contact us this shouldn't happen"
errMultipleAccounts = "multiple accounts found for this address, please contact us this shouldn't happen"
errMultipleAssets = "multiple assets found for this id, please contact us this shouldn't happen"
errMultiAcctRewind = "multiple accounts rewind is not supported by this server"
errRewindingAccount = "error while rewinding account"
errLookingUpBlock = "error while looking up block for round"
errTransactionSearch = "error while searching for transaction"
errSpecialAccounts = "indexer doesn't support fee sink and rewards pool accounts, please refer to algod for relevant information"
errFailedLoadSpecialAccounts = "failed to retrieve special accounts"
errZeroAddressCloseRemainderToRole = "searching transactions by zero address with close address role is not supported"
errZeroAddressAssetSenderRole = "searching transactions by zero address with asset sender role is not supported"
errZeroAddressAssetCloseToRole = "searching transactions by zero address with asset close address role is not supported"
)

var errUnknownAddressRole string
Expand Down
58 changes: 57 additions & 1 deletion api/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net/http"
"strconv"
"strings"

"github.com/algorand/go-algorand/data/basics"
"github.com/labstack/echo/v4"
Expand Down Expand Up @@ -52,6 +53,46 @@ const defaultAssetsLimit = 100
const maxBalancesLimit = 10000
const defaultBalancesLimit = 1000

//////////////////////
// Helper functions //
//////////////////////

func validateTransactionFilter(filter *idb.TransactionFilter) error {
var errorArr = make([]string, 0)

// Round + min/max round
if filter.Round != nil && (filter.MaxRound != 0 || filter.MinRound != 0) {
errorArr = append(errorArr, errInvalidRoundAndMinMax)
}

// If min/max are mixed up
if filter.Round == nil && filter.MinRound != 0 && filter.MaxRound != 0 && filter.MinRound > filter.MaxRound {
errorArr = append(errorArr, errInvalidRoundMinMax)
}

{
var address basics.Address
copy(address[:], filter.Address)
if address.IsZero() {
if filter.AddressRole&idb.AddressRoleCloseRemainderTo != 0 {
errorArr = append(errorArr, errZeroAddressCloseRemainderToRole)
}
if filter.AddressRole&idb.AddressRoleAssetSender != 0 {
errorArr = append(errorArr, errZeroAddressAssetSenderRole)
}
if filter.AddressRole&idb.AddressRoleAssetCloseTo != 0 {
errorArr = append(errorArr, errZeroAddressAssetCloseToRole)
}
}
}

if len(errorArr) > 0 {
return errors.New("invalid input: " + strings.Join(errorArr, ", "))
}

return nil
}

////////////////////////////
// Handler implementation //
////////////////////////////
Expand Down Expand Up @@ -274,13 +315,18 @@ func (si *ServerImplementation) LookupApplicationLogsByID(ctx echo.Context, appl
MinRound: params.MinRound,
MaxRound: params.MaxRound,
Address: params.SenderAddress,
AddressRole: strPtr(addrRoleSender),
}

filter, err := transactionParamsToTransactionFilter(searchParams)
if err != nil {
return badRequest(ctx, err.Error())
}
filter.AddressRole = idb.AddressRoleSender
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍


err = validateTransactionFilter(&filter)
if err != nil {
return badRequest(ctx, err.Error())
}

// Fetch the transactions
txns, next, round, err := si.fetchTransactions(ctx.Request().Context(), filter)
Expand Down Expand Up @@ -457,6 +503,11 @@ func (si *ServerImplementation) LookupTransaction(ctx echo.Context, txid string)
return badRequest(ctx, err.Error())
}

err = validateTransactionFilter(&filter)
if err != nil {
return badRequest(ctx, err.Error())
}

// Fetch the transactions
txns, _, round, err := si.fetchTransactions(ctx.Request().Context(), filter)
if err != nil {
Expand Down Expand Up @@ -487,6 +538,11 @@ func (si *ServerImplementation) SearchForTransactions(ctx echo.Context, params g
return badRequest(ctx, err.Error())
}

err = validateTransactionFilter(&filter)
if err != nil {
return badRequest(ctx, err.Error())
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This should go into transactionParamsToTransactionFilter or si.fetchTransactions to make sure the constraint is checked for all account searches.

Copy link
Contributor Author

@tolikzinovyev tolikzinovyev Nov 8, 2021

Choose a reason for hiding this comment

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

Only LookupApplicationLogsByID() violates, which I think is fine. If we do put it in fetchTransactions(), LookupApplicationLogsByID() would return a weird error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize that this doesn't happen in practice, but all of the other parameter checking happens in transactionParamsToTransactionFilter so it would be good to keep that together.

The fact that the app log search violates this constraint is pretty weird, but do you think it's a problem to generate an error here? The zero address probably shouldn't be making application calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

transactionParamsToTransactionFilter() is a conversion function, I don't want to put business logic there.

I think the current code is ok, especially given that no one in their right mind will use LookupApplicationLogsByID() with zero address.

Copy link
Contributor

@winder winder Nov 8, 2021

Choose a reason for hiding this comment

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

It also validates the input, there's a test in api/handlers_test.go for it also so you can add a test:

    {
      name: "Invalid zero address search",
      params: generated.SearchForTransactionsParams{
        Address:     strPtr(basics.Address{}.String()),
        AddressRole: 9,
      },
      filter:        idb.TransactionFilter{},
      errorContains: []string{errInvalidZeroAddressSearch},
    }

Could you also add the error message to error_messages.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the error message to error_messages.go. transactionParamsToTransactionFilter() does a couple of sanity checks, but no more. Definitely no artificial limitations should be in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is incorrect that

AddressRole: strPtr(addrRoleSender),
becomes idb.AddressRoleSender | idb.AddressRoleAssetSender, so I set filter.AddressRole directly and run validation that used to be in transactionParamsToTransactionFilter() after.

// Fetch the transactions
txns, next, round, err := si.fetchTransactions(ctx.Request().Context(), filter)
if err != nil {
Expand Down
92 changes: 72 additions & 20 deletions api/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/labstack/echo/v4"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"

"github.com/algorand/go-algorand-sdk/encoding/msgpack"
"github.com/algorand/go-algorand/data/basics"
Expand Down Expand Up @@ -136,22 +137,6 @@ func TestTransactionParamToTransactionFilter(t *testing.T) {
},
nil,
},
{
name: "Round + Min/Max Error",
params: generated.SearchForTransactionsParams{
Round: uint64Ptr(10),
MinRound: uint64Ptr(5),
MaxRound: uint64Ptr(15),
},
filter: idb.TransactionFilter{},
errorContains: []string{errInvalidRoundAndMinMax},
},
{
name: "Swapped Min/Max Round",
params: generated.SearchForTransactionsParams{MinRound: uint64Ptr(20), MaxRound: uint64Ptr(10)},
filter: idb.TransactionFilter{},
errorContains: []string{errInvalidRoundMinMax},
},
{
name: "Illegal Address",
params: generated.SearchForTransactionsParams{Address: strPtr("Not-our-base32-thing")},
Expand Down Expand Up @@ -215,23 +200,90 @@ func TestTransactionParamToTransactionFilter(t *testing.T) {
}

for _, test := range tests {
//test := test
t.Run(test.name, func(t *testing.T) {
//t.Parallel()
filter, err := transactionParamsToTransactionFilter(test.params)
if test.errorContains != nil {
if len(test.errorContains) > 0 {
require.Error(t, err)
for _, msg := range test.errorContains {
assert.Contains(t, err.Error(), msg)
}
} else {
assert.NoError(t, err)
assert.Equal(t, test.errorContains != nil, err != nil)
assert.Equal(t, test.filter, filter)
}
})
}
}

func TestValidateTransactionFilter(t *testing.T) {
tests := []struct {
name string
filter idb.TransactionFilter
errorContains []string
}{
{
"Default",
idb.TransactionFilter{Limit: defaultTransactionsLimit},
nil,
},
{
name: "Round + MinRound Error",
filter: idb.TransactionFilter{
Round: uint64Ptr(10),
MaxRound: 15,
},
errorContains: []string{errInvalidRoundAndMinMax},
},
{
name: "Round + MinRound Error",
filter: idb.TransactionFilter{
Round: uint64Ptr(10),
MinRound: 5,
},
errorContains: []string{errInvalidRoundAndMinMax},
},
{
name: "Swapped Min/Max Round",
filter: idb.TransactionFilter{
MinRound: 15,
MaxRound: 5,
},
errorContains: []string{errInvalidRoundMinMax},
},
{
name: "Zero address close address role",
filter: idb.TransactionFilter{
Address: addrSlice(basics.Address{}),
AddressRole: idb.AddressRoleSender | idb.AddressRoleCloseRemainderTo,
},
errorContains: []string{errZeroAddressCloseRemainderToRole},
},
{
name: "Zero address asset sender and asset close address role",
filter: idb.TransactionFilter{
Address: addrSlice(basics.Address{}),
AddressRole: idb.AddressRoleAssetSender | idb.AddressRoleAssetCloseTo,
},
errorContains: []string{
errZeroAddressAssetSenderRole, errZeroAddressAssetCloseToRole},
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
err := validateTransactionFilter(&test.filter)
if len(test.errorContains) > 0 {
require.Error(t, err)
for _, msg := range test.errorContains {
assert.Contains(t, err.Error(), msg)
}
} else {
assert.NoError(t, err)
}
})
}
}

func loadResourceFileOrPanic(path string) []byte {
data, err := ioutil.ReadFile(path)
if err != nil {
Expand Down
6 changes: 6 additions & 0 deletions api/pointer_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,3 +101,9 @@ func strArrayPtr(x []string) *[]string {
}
return &x
}

func addrSlice(x basics.Address) []byte {
xx := new(basics.Address)
*xx = x
return xx[:]
}
6 changes: 0 additions & 6 deletions idb/postgres/internal/writer/writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,6 @@ func getTransactionParticipants(stxnad *transactions.SignedTxnWithAD, includeInn
// if no inner transactions then adding into a slice with in-place de-duplication
res := make([]basics.Address, 0, acctsPerTxn)
add := func(address basics.Address) {
if address.IsZero() {
return
}
for _, p := range res {
if address == p {
return
Expand All @@ -295,9 +292,6 @@ func getTransactionParticipants(stxnad *transactions.SignedTxnWithAD, includeInn
size := acctsPerTxn * (1 + len(stxnad.ApplyData.EvalDelta.InnerTxns)) // approx
participants := make(map[basics.Address]struct{}, size)
add := func(address basics.Address) {
if address.IsZero() {
return
}
participants[address] = struct{}{}
}

Expand Down