bug-fix: include currency-greater-than param for 0 value

[shiqizng]

This PR fixes #810

[algochoi]

This looks fine, but I don't think it completely closes #414 -- it still uses the '0' string workaround, and I wonder if we can just change the behavior of removeFalsyOrEmpty instead:
https://github.com/algorand/js-algorand-sdk/blob/develop/src/client/client.ts#L26

I'm not sure if this would affect the messagepack serialization behavior though.

[shiqizng]

This looks fine, but I don't think it completely closes #414 -- it still uses the '0' string workaround, and I wonder if we can just change the behavior of removeFalsyOrEmpty instead: https://github.com/algorand/js-algorand-sdk/blob/develop/src/client/client.ts#L26

I'm not sure if this would affect the messagepack serialization behavior though.

I don't see a way to change removeFalsyOrEmpty so it allows 0 value without adding a special case for key currency-greater-than.

@algorand/lamprey what do others think about this? should we use a string work around or add a special handling in removeFalsyOrEmpty for individual fields?

[algochoi]

I don't see a way to change removeFalsyOrEmpty so it allows 0 value without adding a special case for key currency-greater-than.

I also don't see a very easy way to fix #414 without affecting serialization and breaking all our current tests.

It seems potentially ugly, but I'd actually be in favor of what you suggested (having special cases). I think it's more obvious and self-documenting that this is a weird workaround in the JS SDK. As is, I don't think it's obvious to devs why we need to pass '0's around without looking at this open issue (414), and especially if we have future client parameters that may require 0 as a valid parameter.

[jasonpaulos]

It is possible we could stop running queries through the removeFalsyOrEmpty function? I'm not sure what benefits we even get from this. I'm pretty sure Python does not do something similar, and it certainly works.

[shiqizng]

It is possible we could stop running queries through the removeFalsyOrEmpty function? I'm not sure what benefits we even get from this. I'm pretty sure Python does not do something similar, and it certainly works.

If I remove this function, then the path seem to include accept=application%2Fjson which is causing the tests to fail.

✖ Then expect the path used to be "/v2/blocks/3?header-only=true" # tests/cucumber/steps/index.js:513
         AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
         + actual - expected

         + '/v2/blocks/3?accept=application%2Fjson'
         - '/v2/blocks/3?header-only=true'
                         ^
             + expected - actual

             -/v2/blocks/3?accept=application%2Fjson
             +/v2/blocks/3?header-only=true

[jasonpaulos]

That's very strange, I have no idea where it's getting accept=application%2Fjson from... 🤔

[bbroder-algo]

I think I would want to know what falsy values or values with length zero we are removing from queries, and whether it can be removed or replaced with alternate logic. my hunch is the accept=application%2Fjson situation can be resolved.

[shiqizng]

I think I would want to know what falsy values or values with length zero we are removing from queries, and whether it can be removed or replaced with alternate logic. my hunch is the accept=application%2Fjson situation can be resolved.

This is to be addressed in #414

[algochoi]

Agree with @bbroder-algo that we need to change or remove that comment on the client method -- Otherwise LGTM, and I don't have a strong opinion on whether we need to address that falsy problem in this PR.

[algochoi]

Approving - optional nit regarding adding comments for converting params to string