Add query for the total supply of a coin #1356

larry0x · 2022-07-12T17:27:28Z

currently it is not possible to query the total supply of a native SDK coin. this is something that protocols often need to do, especially that now contract are able to mint native coins thanks to the tokenfactory module. this PR adds the binding for this query type.

see also:

ueco-jb

Looks good, although you need to include tests for that query_supply helper because otherwise coverage won't pass it through.

larry0x · 2022-07-13T12:21:39Z

Looks good, although you need to include tests for that query_supply helper because otherwise coverage won't pass it through.

@ueco-jb thanks, will do. i did not include it because other bank query helpers (e.g. query_balance) don't have corresponding unit tests either

webmaster128

Very nice. Could you add an entry to the Unreleased section of the CHANGELOG.md?

We need to think about more about how this breaks users but overall it looks solid.

packages/std/src/query/bank.rs

webmaster128 · 2022-07-13T12:45:19Z

packages/std/src/query/bank.rs

+#[serde(rename_all = "snake_case")]
+pub struct SupplyResponse {
+    /// Always returns a Coin with the requested denom.
+    /// This may be of 0 amount if no such funds.


Do we also get 0 in case the denom does not exist at all? Or would that be an error. I think we should specify that.

we get a zero, because this is what BankKeeper.GetSupply does. will add a sentence on this

for reference: https://github.com/cosmos/cosmos-sdk/blob/v0.45.6/x/bank/keeper/keeper.go#L222-L228

Co-authored-by: Simon Warta <[email protected]>

larry0x · 2022-07-14T01:16:57Z

@webmaster128 pls see the changes made according to your comments

packages/std/src/query/bank.rs

webmaster128

Nice work, thank you!

The only question left for me is what happens to users that use 1.1 contracts with this query on 1.0 chains that don't have the implementation. Their contracts will fail at runtime.

Given that it will take a while until contract devs pick up the new query and chains are hungy for upgrades, it does not seem to be a big issue. Also people thoroughly test their contracts on a testnet, right? 😉

We could add a feature flag for this but it would be heavy lifting. Also FEATURES.md documents

A good feature makes sense to be disabled. [...]
When functionality is always present in the VM (such as a new import implemented
directly in the VM, see [#1299]), we should not use features. They just create
fragmentation in the CosmWasm ecosystem and increase the barrier for adoption.
Instead the check_wasm_imports check is used to validate this when the
contract is stored.

While the motivation to not create a new feature here is valid, we don't have a check in place when uploading the contract.

ueco-jb · 2022-07-14T07:11:51Z

Their contracts will fail at runtime.

Shouldn't this PR be marked as breaking then and released with minor?

webmaster128 · 2022-07-14T07:22:10Z

Shouldn't this PR be marked as breaking then and released with minor?

The next release will be 1.1, so there is some signaling in the version number. Regarding "breaking", that can mean so many things in our context. This PR will not break source code or existing compiled constracts and will not affact the vast majority of users. Also there is nothing wrong with failing at runtime when a new query is used the chain does not yet support. It's just not the most convenient thing possible.

ueco-jb · 2022-07-14T08:45:38Z

Alright. In that case I'd maybe at least add some disclaimer, that it will fail at runtime for older versions.

larry0x · 2022-07-14T08:55:37Z

The only question left for me is what happens to users that use 1.1 contracts with this query on 1.0 chains that don't have the implementation. Their contracts will fail at runtime.

so the osmo-bindings library asserts that any contract importing it can only be run on the osmosis chain by having this line: https://github.com/osmosis-labs/bindings/blob/v0.6.0/packages/bindings/src/lib.rs#L11-L14

i'm wondering if it's possible to assert wasmd version the same way?

webmaster128 · 2022-07-14T08:59:23Z

The marker export extern "C" fn requires_* is one part of the features mechanism described in https://github.com/CosmWasm/cosmwasm/blob/main/docs/FEATURES.md.

It would be a solution. But if we start using the features mechanism for all those small incremental improvements that are not meant to be deactivated long term, then we pollute the features list a lot.

webmaster128 · 2022-07-16T08:58:46Z

This is the best approach to compatibility I can think of right now. If you disagree, feel free to continue the discussion in that ticket.

From my side this PR is good to get merged. Can you resolve the conflicts?

larry0x · 2022-07-29T00:53:03Z

This is the best approach to compatibility I can think of right now. If you disagree, feel free to continue the discussion in that ticket.

From my side this PR is good to get merged. Can you resolve the conflicts?

hi @webmaster128 sorry for the lack of actions on this PR. have been incredibly busy the last 2 weeks.

the conflicts have been resolved, but i see there still isn't a clear agreement in the discussion #1367

this being said, can you also review these two PRs, which are prerequisites for this one to work:

webmaster128 · 2022-08-17T12:31:01Z

Hey Larry, after some internal discussions we concluded that this case fits well into the capabilities concept and we can create a cosmwasm_1_1 capability. This makes it possible to ship this work in the next release.

I guess the reflect contract can be used to test this new query (QueryMsg::Chain). But not sure.

webmaster128 · 2022-08-23T13:33:20Z

@larry0x Tom is taking over this PR if you don't mind. Thanks again.

uint · 2022-08-23T13:39:12Z

I was just updating this PR and resolving conflicts; I'll try to add the capability stuff in a separate PR targeting this one

larry0x · 2022-08-23T13:47:37Z

Hi @uint @webmaster128 i certainly don't mind. Tbh i don't know what's the approach Confio is taking towards breaking API changes like this, so it's better you guys handle it. Please be reminded that there are two other PRs in wasmvm and wasmd which are prerequisites for this one to work.

uint · 2022-08-23T14:03:44Z

Alright, the "PR into this PR" option doesn't seem feasible. I'll just push here. Thanks, @larry0x!

uint · 2022-08-23T17:23:48Z

This probably needs updating too:
https://github.com/CosmWasm/cosmwasm/blob/main/packages/std/src/mock.rs#L455

I'll figure it out tomorrow.

larry0x added 3 commits July 12, 2022 16:20

add bank supply query

9de7624

export SupplyResponse

eb66d7a

add a helper function for querying supply

de275e8

This was referenced Jul 12, 2022

Add query for the total supply of a coin CosmWasm/wasmvm#337

Merged

Add query for the total supply of a coin CosmWasm/wasmd#903

Merged

larry0x added 3 commits July 12, 2022 19:35

update schema

ddd4e3c

update import

787cef0

fix import

ce2577b

ueco-jb reviewed Jul 13, 2022

View reviewed changes

webmaster128 reviewed Jul 13, 2022

View reviewed changes

larry0x and others added 12 commits July 14, 2022 00:29

add unit test for bank query helpers

c1e0b78

add an entry to changelog

0293397

Update packages/std/src/query/bank.rs

ecb68ea

Co-authored-by: Simon Warta <[email protected]>

clarify in comment the case if a denom doesn not exist

11cd9a5

add a unit test for querying the supply of a non-existing denom

7d47853

revert changes made to Cargo.toml (not supposed to be pushed)

d97d68b

remove a contract unintentionally pushed (only used for local testing)

f15a25f

format changelog

21e505c

revert unintential changes made to Cargo.toml

032c34c

update schema

2aeca60

resolve conflict

2cd39d1

updat schema

6f125e4

webmaster128 reviewed Jul 14, 2022

View reviewed changes

packages/std/src/query/bank.rs Show resolved Hide resolved

Update packages/std/src/query/bank.rs

abe4a4f

webmaster128 approved these changes Jul 14, 2022

View reviewed changes

webmaster128 added this to the 1.1.0 milestone Jul 16, 2022

webmaster128 mentioned this pull request Jul 16, 2022

Create new interface version #1367

Closed

3 tasks

Merge branch 'main' into larry/bank-query-total-supply

0e619bc

uint removed this from the 1.1.0 milestone Aug 9, 2022

webmaster128 added this to the 1.1.0 milestone Aug 17, 2022

webmaster128 mentioned this pull request Aug 22, 2022

Create cosmwasm_1_1 capability #1399

Closed

uint added 3 commits August 23, 2022 13:48

Merge branch 'main' into larry/bank-query-total-supply

ef952c3

fix poorly resolved merge conflict

162bcfd

hide BankQuery::Supply behind the cosmwasm_1_1 feature

61bcb49

uint added 2 commits August 23, 2022 15:43

fix Rust 1.63 lints

914f87c

generate std schemas including cosmwasm_1_1 feature

b440266

test BankQuery::Supply using the reflect contract

3cdc9ef

uint added 2 commits August 23, 2022 19:29

update CI after introducing the cosmwasm_1_1 feature

74a68bc

avoid BankQuerier.supplies ending up with invalid state

492e6d7

uint requested a review from webmaster128 August 24, 2022 12:05

webmaster128 approved these changes Aug 29, 2022

View reviewed changes

uint merged commit a9ae6fa into CosmWasm:main Aug 29, 2022

da1suk8 mentioned this pull request Oct 17, 2022

Check the changes of cosmwasm/wasmd v0.29 Finschia/finschia-sdk#684

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add query for the total supply of a coin #1356

Add query for the total supply of a coin #1356

larry0x commented Jul 12, 2022 •

edited

Loading

ueco-jb left a comment

larry0x commented Jul 13, 2022

webmaster128 left a comment

webmaster128 Jul 13, 2022

larry0x Jul 13, 2022

larry0x Jul 13, 2022

larry0x commented Jul 14, 2022

webmaster128 left a comment

ueco-jb commented Jul 14, 2022 •

edited

Loading

webmaster128 commented Jul 14, 2022 •

edited

Loading

ueco-jb commented Jul 14, 2022

larry0x commented Jul 14, 2022

webmaster128 commented Jul 14, 2022 •

edited

Loading

webmaster128 commented Jul 16, 2022 •

edited

Loading

larry0x commented Jul 29, 2022

webmaster128 commented Aug 17, 2022

webmaster128 commented Aug 23, 2022

uint commented Aug 23, 2022

larry0x commented Aug 23, 2022

uint commented Aug 23, 2022

uint commented Aug 23, 2022

Add query for the total supply of a coin #1356

Add query for the total supply of a coin #1356

Conversation

larry0x commented Jul 12, 2022 • edited Loading

ueco-jb left a comment

Choose a reason for hiding this comment

larry0x commented Jul 13, 2022

webmaster128 left a comment

Choose a reason for hiding this comment

webmaster128 Jul 13, 2022

Choose a reason for hiding this comment

larry0x Jul 13, 2022

Choose a reason for hiding this comment

larry0x Jul 13, 2022

Choose a reason for hiding this comment

larry0x commented Jul 14, 2022

webmaster128 left a comment

Choose a reason for hiding this comment

ueco-jb commented Jul 14, 2022 • edited Loading

webmaster128 commented Jul 14, 2022 • edited Loading

ueco-jb commented Jul 14, 2022

larry0x commented Jul 14, 2022

webmaster128 commented Jul 14, 2022 • edited Loading

webmaster128 commented Jul 16, 2022 • edited Loading

larry0x commented Jul 29, 2022

webmaster128 commented Aug 17, 2022

webmaster128 commented Aug 23, 2022

uint commented Aug 23, 2022

larry0x commented Aug 23, 2022

uint commented Aug 23, 2022

uint commented Aug 23, 2022

larry0x commented Jul 12, 2022 •

edited

Loading

ueco-jb commented Jul 14, 2022 •

edited

Loading

webmaster128 commented Jul 14, 2022 •

edited

Loading

webmaster128 commented Jul 14, 2022 •

edited

Loading

webmaster128 commented Jul 16, 2022 •

edited

Loading