Support search by zero address. #771

tolikzinovyev · 2021-11-04T18:47:29Z

Summary

Zero address is a real address and we shouldn't ignore it. People are curious about transactions in which the zero address participates.

winder

This looks good, 2 requests for before you merge:

Rename the commit with the feature being added so that the release notes are easier to write: Support search by zero address.
Add comments for each switch case about why zero is only allowed sometimes. I'm pretty sure you got them all correct, but these are subtle details so things comments like "AssetSender is optional, non-zero values indicate a clawback transaction" may help the next person reading this code.

tolikzinovyev · 2021-11-05T01:24:24Z

Thanks, done. I wonder if we need to check for zero address in buildTransactionQuery(). For example, it doesn't make sense to set address to zero and address role to AddressRoleCloseRemainderTo in TransactionFilter. Should we trust the user to construct the right request?

codecov-commenter · 2021-11-05T01:36:30Z

Codecov Report

Merging #771 (372c50d) into develop (7dff12e) will decrease coverage by 0.15%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop     #771      +/-   ##
===========================================
- Coverage    54.47%   54.31%   -0.16%     
===========================================
  Files           28       28              
  Lines         3870     3874       +4     
===========================================
- Hits          2108     2104       -4     
- Misses        1481     1489       +8     
  Partials       281      281

Impacted Files	Coverage Δ
accounting/accounting.go	`0.00% <0.00%> (ø)`
idb/postgres/internal/writer/writer.go	`76.63% <ø> (-0.43%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dff12e...372c50d. Read the comment docs.

winder · 2021-11-05T13:33:26Z

Thanks, done. I wonder if we need to check for zero address in buildTransactionQuery(). For example, it doesn't make sense to set address to zero and address role to AddressRoleCloseRemainderTo in TransactionFilter. Should we trust the user to construct the right request?

In that case, we'd just return no results. Technically correct, right? I don't think we need to bother with custom error messages.

tolikzinovyev · 2021-11-05T17:23:05Z

Thanks, done. I wonder if we need to check for zero address in buildTransactionQuery(). For example, it doesn't make sense to set address to zero and address role to AddressRoleCloseRemainderTo in TransactionFilter. Should we trust the user to construct the right request?

In that case, we'd just return no results. Technically correct, right? I don't think we need to bother with custom error messages.

No. Suppose the query is Address = zero, Role = CloseAddress. It will find a transaction where receiver is zero and close address is zero. But it will not find a transaction where receiver is non-zero and close address is zero. We could suppose the user should not make such a query. Any thoughts?

winder · 2021-11-05T19:35:05Z

Thanks, done. I wonder if we need to check for zero address in buildTransactionQuery(). For example, it doesn't make sense to set address to zero and address role to AddressRoleCloseRemainderTo in TransactionFilter. Should we trust the user to construct the right request?

In that case, we'd just return no results. Technically correct, right? I don't think we need to bother with custom error messages.

No. Suppose the query is Address = zero, Role = CloseAddress. It will find a transaction where receiver is zero and close address is zero. But it will not find a transaction where receiver is non-zero and close address is zero. We could suppose the user should not make such a query. Any thoughts?

I'm pretty this is OK. We use the txn_participation table to identify the initial set of transactions, but then have a second filter on the jsonb to match the specific field we're looking for. I guess it might timeout if there are too many transactions li ke this for the slow jsonb filter.

winder · 2021-11-08T14:08:57Z

@tolikzinovyev I thought about this over the weekend and think the simplest solution is to prevent using AddressRole and a zero address. In converter_utils.go : transactionParamsToTransactionFilter this should do it:

	// Don't allow address role + zero address search, there are several invalid configurations.
	if params.AddressRole != nil && params.Address != nil {
		var zeroAddr basics.Address
		if zeroAddr.String() == *params.Address {
			errorArr = append(errorArr, errInvalidZeroAddressSearch)
		}
	}

tolikzinovyev · 2021-11-08T17:01:37Z

@tolikzinovyev I thought about this over the weekend and think the simplest solution is to prevent using AddressRole and a zero address. In converter_utils.go : transactionParamsToTransactionFilter this should do it:
	// Don't allow address role + zero address search, there are several invalid configurations.
	if params.AddressRole != nil && params.Address != nil {
		var zeroAddr basics.Address
		if zeroAddr.String() == *params.Address {
			errorArr = append(errorArr, errInvalidZeroAddressSearch)
		}
	}

Maybe only disallow address role close address, asset sender and asset close address?

winder · 2021-11-08T17:17:35Z

@tolikzinovyev I thought about this over the weekend and think the simplest solution is to prevent using AddressRole and a zero address. In converter_utils.go : transactionParamsToTransactionFilter this should do it:
	// Don't allow address role + zero address search, there are several invalid configurations.
	if params.AddressRole != nil && params.Address != nil {
		var zeroAddr basics.Address
		if zeroAddr.String() == *params.Address {
			errorArr = append(errorArr, errInvalidZeroAddressSearch)
		}
	}
Maybe only disallow address role close address, asset sender and asset close address?

We can make it less strict in the future, for now let's keep it simple

winder

~~LGTM~~

winder

I think the parameter checking should go in a different spot

winder · 2021-11-08T18:01:23Z

api/handlers.go

+				ctx, "searching transactions by zero address with address role is not supported")
+		}
+	}
+


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

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

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.

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.

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?

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.

I think it is incorrect that

indexer/api/handlers.go

Line 277 in 830e1ba

AddressRole: strPtr(addrRoleSender),

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

…lter().

winder · 2021-11-09T01:11:37Z

api/handlers.go

 	}

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


winder

LGTM

tolikzinovyev added Unplanned Team Carbon-11 labels Nov 4, 2021

tolikzinovyev requested review from winder and algorandskiy November 4, 2021 18:47

tolikzinovyev self-assigned this Nov 4, 2021

winder approved these changes Nov 4, 2021

View reviewed changes

tolikzinovyev changed the title ~~Write zero address in txn_participation table~~ Support search by zero address. Nov 5, 2021

winder approved these changes Nov 5, 2021

View reviewed changes

yaovi-a mentioned this pull request Nov 8, 2021

Indexer v2.8.0 full catchup & Validation #774

Closed

2 tasks

tolikzinovyev added 3 commits November 8, 2021 12:36

Write zero address in txn_participation table.

2a2d9cb

Add comments.

2d47eb8

Add a restriction.

c563e99

tolikzinovyev force-pushed the tolik/participation-zero branch from 955fd64 to c563e99 Compare November 8, 2021 17:53

tolikzinovyev requested a review from winder November 8, 2021 17:55

winder approved these changes Nov 8, 2021

View reviewed changes

winder self-requested a review November 8, 2021 17:57

winder suggested changes Nov 8, 2021

View reviewed changes

tolikzinovyev added 3 commits November 8, 2021 15:01

Move error msg to error_messages.go.

f7c7f68

Take out parameter validation out of transactionParamsToTransactionFi…

0c6c56a

…lter().

Run make fmt.

5406b6c

tolikzinovyev requested a review from winder November 8, 2021 23:00

winder reviewed Nov 9, 2021

View reviewed changes

winder approved these changes Nov 9, 2021

View reviewed changes

tolikzinovyev merged commit 1fdbd3f into develop Nov 9, 2021

tolikzinovyev deleted the tolik/participation-zero branch November 9, 2021 16:47

algobarb mentioned this pull request Nov 30, 2021

DO NOT MERGE: indexer 2.8.0 #800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support search by zero address. #771

Support search by zero address. #771

tolikzinovyev commented Nov 4, 2021

winder left a comment •

edited

Loading

tolikzinovyev commented Nov 5, 2021

codecov-commenter commented Nov 5, 2021

winder commented Nov 5, 2021

tolikzinovyev commented Nov 5, 2021

winder commented Nov 5, 2021

winder commented Nov 8, 2021

tolikzinovyev commented Nov 8, 2021

winder commented Nov 8, 2021 •

edited

Loading

winder left a comment •

edited

Loading

winder left a comment

winder Nov 8, 2021

tolikzinovyev Nov 8, 2021 •

edited

Loading

winder Nov 8, 2021

tolikzinovyev Nov 8, 2021

winder Nov 8, 2021 •

edited

Loading

tolikzinovyev Nov 8, 2021

tolikzinovyev Nov 8, 2021

winder Nov 9, 2021

winder left a comment

Support search by zero address. #771

Support search by zero address. #771

Conversation

tolikzinovyev commented Nov 4, 2021

Summary

winder left a comment • edited Loading

Choose a reason for hiding this comment

tolikzinovyev commented Nov 5, 2021

codecov-commenter commented Nov 5, 2021

Codecov Report

winder commented Nov 5, 2021

tolikzinovyev commented Nov 5, 2021

winder commented Nov 5, 2021

winder commented Nov 8, 2021

tolikzinovyev commented Nov 8, 2021

winder commented Nov 8, 2021 • edited Loading

winder left a comment • edited Loading

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

winder Nov 8, 2021

Choose a reason for hiding this comment

tolikzinovyev Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

winder Nov 8, 2021

Choose a reason for hiding this comment

tolikzinovyev Nov 8, 2021

Choose a reason for hiding this comment

winder Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

tolikzinovyev Nov 8, 2021

Choose a reason for hiding this comment

tolikzinovyev Nov 8, 2021

Choose a reason for hiding this comment

winder Nov 9, 2021

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

winder left a comment •

edited

Loading

winder commented Nov 8, 2021 •

edited

Loading

winder left a comment •

edited

Loading

tolikzinovyev Nov 8, 2021 •

edited

Loading

winder Nov 8, 2021 •

edited

Loading