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

feat: (x/bank) add spendable balances cmd #14045

Merged
merged 24 commits into from
Jan 5, 2023

Conversation

facundomedica
Copy link
Member

@facundomedica facundomedica commented Nov 28, 2022

Description

This PR adds a spendable-balances to x/bank's queries.

Questions:

  1. Should we move this command under balances and activate it with a bool flag (--only-spendable)? This would invalidate the --denom flag. If we are ok with that, maybe it's cleaner that way.

Closes: #14030


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@likhita-809
Copy link
Contributor

  1. Should we move this command under balances and activate it with a bool flag (--only-spendable)? This would invalidate the --denom flag. If we are ok with that, maybe it's cleaner that way.

I think spendableCoins uses gas more efficiently when compared to get the balance of an account multiple times after txs.
Maybe I'd prefer having it in a separate command, not to invalidate existing functionality of --denom flag usage.
cc @alexanderbez

@alexanderbez
Copy link
Contributor

  1. Should we move this command under balances and activate it with a bool flag (--only-spendable)? This would invalidate the --denom flag. If we are ok with that, maybe it's cleaner that way.

I think spendableCoins uses gas more efficiently when compared to get the balance of an account multiple times after txs. Maybe I'd prefer having it in a separate command, not to invalidate existing functionality of --denom flag usage. cc @alexanderbez

I like @facundomedica's proposal! Why exactly do you think having a --spendable-only flag is a bad idea?

@likhita-809
Copy link
Contributor

likhita-809 commented Nov 29, 2022

  1. Should we move this command under balances and activate it with a bool flag (--only-spendable)? This would invalidate the --denom flag. If we are ok with that, maybe it's cleaner that way.

I think spendableCoins uses gas more efficiently when compared to get the balance of an account multiple times after txs. Maybe I'd prefer having it in a separate command, not to invalidate existing functionality of --denom flag usage. cc @alexanderbez

I like @facundomedica's proposal! Why exactly do you think having a --spendable-only flag is a bad idea?

I'm not opposed to it and don't really think its a bad idea, but just concerned if we could remove --denom's functionality and its not needed in future, but definitely agree that it makes balances cleaner.

@facundomedica
Copy link
Member Author

So in other words --denom will be ignored if --spendable-only is passed. Another option would be adding a new query that only gets the spendable amount of an specific denom to mimic what GetBalances is doing. Then both --denom and --spendable-only could be used together.

@likhita-809
Copy link
Contributor

So in other words --denom will be ignored if --spendable-only is passed. Another option would be adding a new query that only gets the spendable amount of an specific denom to mimic what GetBalances is doing. Then both --denom and --spendable-only could be used together.

I like this better

@alexanderbez
Copy link
Contributor

@facundomedica well if it's a new query for just spendable balances, then --spendable-only isn't needed anymore and we're back to where we started, no?

@facundomedica
Copy link
Member Author

Coming back to this after a while. I think we have to options:

  1. Modify the current GetBalancesCmd to take in --spendable-only. For this we would need to add a new query like SpendableBalance (And maybe modify SpendableBalances to AllSpendableBalances, but that would be breaking). To mimic Balance and AllBalances, and making possible to get a single denom by passing --denom.
  2. Leave this PR as is, returning a list of spendable balances in a separate command.

If we really want to be able to get a single denom at a time, we should go for 1, if not, I would just stick with 2.

@facundomedica
Copy link
Member Author

So now the spendable-balances accepts the --denom flag, so it works just like the balances cmd.

@alexanderbez
Copy link
Contributor

@facundomedica let's move this to draft mode until it's ready to review again pls.

@sonarcloud
Copy link

sonarcloud bot commented Jan 2, 2023

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
No Duplication information No Duplication information

@facundomedica facundomedica marked this pull request as ready for review January 2, 2023 15:15
Copy link
Contributor

@likhita-809 likhita-809 left a comment

Choose a reason for hiding this comment

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

just one comment, otherwise lgtm
can you also remove the gov expected_keeper_mocks from here and fix these failing tests ?

x/bank/client/cli/query_test.go Show resolved Hide resolved
@@ -47,10 +43,6 @@ func (k BaseKeeper) AllBalances(ctx context.Context, req *types.QueryAllBalances
return nil, status.Error(codes.InvalidArgument, "empty request")
}

if req.Address == "" {
Copy link
Member

Choose a reason for hiding this comment

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

why these removals

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

lef tone question but looks good otherwise

@alexanderbez alexanderbez enabled auto-merge (squash) January 5, 2023 15:50
@alexanderbez
Copy link
Contributor

LGTM. Let's merge.

@alexanderbez alexanderbez merged commit 6ac0c36 into main Jan 5, 2023
@alexanderbez alexanderbez deleted the facu/spendable-balances-cmd branch January 5, 2023 19:58
mergify bot pushed a commit that referenced this pull request Jan 5, 2023
(cherry picked from commit 6ac0c36)

# Conflicts:
#	CHANGELOG.md
julienrbrt added a commit that referenced this pull request Jan 5, 2023
Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release C:CLI C:x/bank C:x/gov
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request to add spendable amount CLI query in bank module
7 participants