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

types: Add AllDenomMetadata BankQuery #409

Closed
wants to merge 2 commits into from

Conversation

nik-suri
Copy link
Contributor

@nik-suri nik-suri commented Mar 24, 2023

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

LGTM. We just need the response type here (see https://github.com/CosmWasm/wasmvm/pull/337/files for an exampe of a new query added to this repo).

@nik-suri
Copy link
Contributor Author

Thanks @webmaster128. Just reposting the same comment I left on my corresponding wasmd PR: CosmWasm/wasmd#1294 (comment)

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
Copy link
Member

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
Copy link
Contributor Author

@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 nik-suri force-pushed the denom-metadata-query branch 2 times, most recently from 4197862 to 4669071 Compare March 29, 2023 22:16
@nik-suri nik-suri force-pushed the denom-metadata-query branch from 4669071 to 3013e81 Compare April 18, 2023 16:25
@nik-suri
Copy link
Contributor Author

nik-suri commented Apr 18, 2023

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

@webmaster128
Copy link
Member

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 webmaster128 added this to the 1.3.0 milestone Apr 18, 2023
@webmaster128
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@webmaster128 Any updates from discussions on your end?

@nik-suri
Copy link
Contributor Author

nik-suri commented Jun 7, 2023

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

@nik-suri nik-suri closed this Jun 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.

2 participants