search

CosmWasm / wasmvm

Go bindings to the running cosmwasm contracts with wasmer
Apache License 2.0
173 stars 99 forks source link

types: Add AllDenomMetadata BankQuery #409

Closed nik-suri closed 1 year ago

nik-suri commented 1 year ago

Please review these related PRs in tandem:

nik-suri commented 1 year ago

Thanks @webmaster128. Just reposting the same comment I left on my corresponding wasmd PR: https://github.com/CosmWasm/wasmd/pull/1294#discussion_r1149752023

This was a way for me to use the same type instead of a copy/pasted type in wasmvm. While digging into the other response types in wasmvm, I saw that they are not used anywhere else within the wasmvm codebase - they are just defined there and then only used by external packages. So, it didn't seem to be necessary to define any of these types in wasmvm.

However, if this is a hard requirement then I'm happy to make the change.

webmaster128 commented 1 year ago

Yeah, this is not super obvious. But the cosmwasm-std/wasmvm communication interface (i.e. the two side of the JSON communication) are all defined here. Maybe we can find a way to enforce more type-safety for wasmd such that the type is actually used here.

nik-suri commented 1 year ago

@webmaster128 added all the types in wasmvm as part of this PR.

Adding these is a bit of a tedious process since we need to

  1. Copy/paste each struct so they're properly redefined
  2. Use conversion methods in the upstream code (see the corresponding wasmd PR)

I am still of the opinion that there's no reason to have these types in wasmvm since they aren't used anywhere else in the codebase. It's also unclear to me why wasmvm and wasmd are in separate packages - I'm not sure what codebases import only wasmvm and not wasmd (and why they would do this).

nik-suri commented 1 year ago

@webmaster128 could you please take another look at this PR and the related ones? I'd love to get these changes merged in!

webmaster128 commented 1 year ago

Absolutely. Plese note that because of https://forum.cosmos.network/t/upcoming-cosmwasm-security-patch-codename-cherry/10474 all CosmWasm 1.3 tickets are currently de-prioritized. We'll pick this up as soon as possible.

webmaster128 commented 1 year ago

We are currently discussing internally how the pagination should be handled. So far we do not expose any API with pagination to smart contract developers.

How many entries do you expect in the chains that you are working with?

nik-suri commented 1 year ago

We are currently discussing internally how the pagination should be handled. So far we do not expose any API with pagination to smart contract developers.

How many entries do you expect in the chains that you are working with?

@webmaster128 it would be any number - we'd like this cosmwasm API to work with any chain that supports cosmwasm.

Not all chains have denom metadata populated, but some have a lot (@alpe mentioned that Osmosis has >970 entries).

Some chains also have implemented token factories, which means there could be a very high number of denom-metadata entries.

nik-suri commented 1 year ago

@webmaster128 Any updates from discussions on your end?

nik-suri commented 1 year ago

Closing in favor of #430. Thank you @chipshort!