currencyGreaterThan or currencyLessThan should appear in query if the value is explicitly set to 0

[fionnachan]

Subject of the issue

Various attempts were made to fix algorand/js-algorand-sdk#414 but the PRs were blocked by failed cucumber tests which have an inaccurate expected behaviour for the feature.

See below

PR author	PR	CircleCI test report
@AlgoDoggo	algorand/js-algorand-sdk#562	https://app.circleci.com/pipelines/github/algorand/js-algorand-sdk/158/workflows/9878838e-a782-48dc-89c6-0a74df888b9d/jobs/794
@barnjamin	algorand/js-algorand-sdk#571	https://app.circleci.com/pipelines/github/algorand/js-algorand-sdk/180/workflows/971cbb96-78b7-4a19-91d2-1b17f4900151/jobs/925

Expected behaviour

These are the expected behaviour for the SDK.

1. [working as expected] When currencyGreaterThan or currencyLessThan is unset, it should not appear in the url query list.
2. [working as expected] When currencyLessThan is set to 0, the returned results should be empty or throw an error for disallowed value.
3. [working as expected] When currencyGreaterThan is set to 0, the returned result from the API is different from that of when currencyGreaterThan is unset.
   => And that's why currency-greater-than=0 should be in the query list if 0 is set explicitly.
4. [not working] When currencyGreaterThan or currencyLessThan is set to 0, it should appear in the url query list.

Actual behaviour

There are many tests in https://github.com/algorand/algorand-sdk-testing/blob/master/features/unit/v2indexerclient_paths.feature that has the same inaccurate expectation. Below is one of the examples:
currency-greater-than=0 or currency-less-than=0 is expected to not appear in the url's query list by the cucumber tests even though 0 is being explicitly set.

algorand-sdk-testing/features/unit/v2indexerclient_paths.feature

Lines 6 to 15 in d60f7b5

	@unit.indexer
	Scenario Outline: LookupAssetBalances path
	When we make a Lookup Asset Balances call against asset index <index> with limit <limit> afterAddress "<afterAddress>" currencyGreaterThan <currencyGreaterThan> currencyLessThan <currencyLessThan>
	Then expect the path used to be "<path>"
	Examples:
	| path | index | limit | currencyGreaterThan | currencyLessThan | afterAddress |
	| /v2/assets/100/balances | 100 | 0 | 0 | 0 | |
	| /v2/assets/100/balances?limit=1 | 100 | 1 | 0 | 0 | |
	| /v2/assets/100/balances?currency-greater-than=3 | 100 | 0 | 3 | 0 | |
	| /v2/assets/100/balances?currency-less-than=4 | 100 | 0 | 0 | 4 | |

[winder]

I don't think the server supports setting values to 0, so this bug is more than an SDK/test quirk. The tests are actually enforcing the current server expectations.

[AlgoDoggo]

@winder how do you mean? currency-greater-than=0 works as a query string, it accurately filters out accounts who hold 0 of the asset. It only doesn't work with the algosdk but I use it regularly when consuming the indexer api with other libs.

[fionnachan]

@winder
USDC, currency-greater-than=0:
https://algoindexer.algoexplorerapi.io/v2/assets/31566704/balances?currency-greater-than=0
This excludes accounts with 0 balance

USDC, currency-greater-than unset:
https://algoindexer.algoexplorerapi.io/v2/assets/31566704/balances
This includes accounts with 0 balance

But the SDK would not allow the first case currency-greater-than=0 to be queried

[barnjamin]

This is definitely a bug in at least the JS SDK

This section removes valid zeros
https://github.com/algorand/js-algorand-sdk/blob/18b863be6f4fe435b77c75b62316c0a599d88d68/src/client/client.ts#L25-L33

The work around currently is just to set a string "0" so that it makes it through the checks.

This PR I filed last year was to allow searching valid 0s in the indexer: algorand/indexer#484

[winder]

Thanks @barnjamin, I forgot about that! I suppose the testing infrastructure and the SDKs would both need to account for this.

[fionnachan]

Those JS PRs have been sitting there for almost 3 months now.... 👀

[algoanne]

SDKs need to be able to send 0 for query parameters. There are several transaction endpoints that have this issue/capability.

To solve this, we need to:

1. audit our endpoints to make sure we solve it for all affected ones.
2. update our cucumber tests for each affected endpoint.
3. update each SDK correspondingly (likely all 4 SDKs, but need to confirm that the behavior appears in the Go and Java SDKs as well).

we will complete (1) and (2) as part of this ticket, then (3) with individual tickets for each SDK:

• python: Faulty check in indexer_client.asset_balances method makes it impossible to get balances grater than 0. py-algorand-sdk#296
• JS: Remove function removeFalsyOrEmpty js-algorand-sdk#414