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

Added None checking in min_balance validation #271

Closed
wants to merge 4 commits into from

Conversation

razmikarm
Copy link

Added None checking in min_balance validating condition so the user can look up for balances with more than 0 amount.

@CLAassistant
Copy link

CLAassistant commented Jan 4, 2022

CLA assistant check
All committers have signed the CLA.

@razmikarm
Copy link
Author

Can make more changes like this for keeping general coding style, if you approve this changes.

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.

This looks like a good change, thank you! Please feel free to add additional checks.

Also I think you might have to sign the CLA for this to be checked in.

algochoi
algochoi previously approved these changes Mar 15, 2022
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.

Let's merge this after tests pass...

edit: It looks like the tests were written without this query in mind - we'd also have to change the existing tests to make this pass the CI so we'll have to hold this off

@jasonpaulos
Copy link
Contributor

Unfortunately this is a more systemic issue. The tests here expect that currencyGreaterThan=0 does not include that parameter in the request: https://github.com/algorand/algorand-sdk-testing/blob/6bcf84603677e945dbca176e8e2870a9adca22fd/features/unit/v2indexerclient_paths.feature#L92-L97

We could decide to switch this behavior, but that would affect all the SDKs.

Related to algorand/js-algorand-sdk#414 as well

@algochoi
Copy link
Contributor

algochoi commented Aug 7, 2023

#507 supersedes this PR.

@algochoi algochoi closed this Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants