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

Exclude IBC assets from TotalSupply if their denom metadata is nonexistent #4518

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

zbuc
Copy link
Member

@zbuc zbuc commented May 31, 2024

Describe your changes

This changes the TotalSupply query to skip including assets when there is no denom metadata available, for example liquidity position NFTs. This is okay since these are mostly of interest to a single user rather than the market as a whole.

I validated that the error when querying on the current v0.77.0 testnet goes away.

$ grpcurl -plaintext -vv localhost:8080 cosmos.bank.v1beta1.Query/TotalSupply

Issue ticket number and link

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    Changes RPC output only

@cratelyn cratelyn added this to the Sprint 7 milestone Jun 3, 2024
Copy link
Member

@erwanor erwanor 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 good to me, we can do a round of improvement later as part of #4446

@erwanor erwanor added A-IBC Area: IBC integration with Penumbra C-bug Category: a bug labels Jun 3, 2024
@zbuc zbuc merged commit 785ac6a into main Jun 3, 2024
13 checks passed
@zbuc zbuc deleted the fix_totalsupply branch June 3, 2024 16:35
conorsch pushed a commit that referenced this pull request Jun 3, 2024
…stent (#4518)

## Describe your changes

This changes the `TotalSupply` query to skip including assets when there
is no denom metadata available, for example liquidity position NFTs.
This is okay since these are mostly of interest to a single user rather
than the market as a whole.

I validated that the error when querying on the current v0.77.0 testnet
goes away.

```
$ grpcurl -plaintext -vv localhost:8080 cosmos.bank.v1beta1.Query/TotalSupply
```

## Issue ticket number and link

## Checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > Changes RPC output only

(cherry picked from commit 785ac6a)
conorsch pushed a commit that referenced this pull request Jun 3, 2024
…stent (#4518)

## Describe your changes

This changes the `TotalSupply` query to skip including assets when there
is no denom metadata available, for example liquidity position NFTs.
This is okay since these are mostly of interest to a single user rather
than the market as a whole.

I validated that the error when querying on the current v0.77.0 testnet
goes away.

```
$ grpcurl -plaintext -vv localhost:8080 cosmos.bank.v1beta1.Query/TotalSupply
```

## Issue ticket number and link

## Checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > Changes RPC output only

(cherry picked from commit 785ac6a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-IBC Area: IBC integration with Penumbra C-bug Category: a bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants