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!: Adds all stargate queries for smart contracts #715

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Reecepbcups
Copy link
Contributor

@Reecepbcups Reecepbcups commented Jun 16, 2023

Closes #714

TODO:

  • add go test and/or e2e test with ic for EVERY one of these
  • lint

Comment on lines +721 to +723
for k, v := range std {
updated[k] = v
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
@dimiandre
Copy link
Member

I'm hyped but also scared

We need to double check event returns to be ordered and determinism of each of these

@Reecepbcups
Copy link
Contributor Author

@dimiandre totally agree - I want to have a contract / e2e test which checks all of this with -race across 8+ vals on a network to ensure its safe before merge

@Reecepbcups
Copy link
Contributor Author

Reecepbcups commented Jun 21, 2023

@mikedotexe Are you able to work on a contract for this? You have more rustc proto experience than I do - or maybe a contract which uses Nois's Anybuf?


Guidelines if so:

Run this repo with the instructions in ./scripts/test_node.sh at the top in this branch

Then I can build a e2e test with 8 validators so we can ensure the chain does not halt for non-determinism.

@mikedotexe
Copy link
Contributor

Are you able to work on a contract for this?

Hey! My honest answer is I will probably need to reduce my time after AwesomWasm and OsmoCon to catch up on energy I've lost during my CosmWasm marathon. Maybe we can chat about this in late August?

@mikedotexe
Copy link
Contributor

I'm hyped but also scared

I feel the same way. If it's preferable, perhaps we can consider adding a handful of Stargate queries that might unlock a lot of potential. I know Yieldmos is keen on having Authz and FeeGrant queries going. Those might be worthwhile to get in before a larger, blanket acceptance of all Stargate queries?

@Reecepbcups
Copy link
Contributor Author

Reecepbcups commented Jun 28, 2023

accepted := wasmkeeper.AcceptedStargateQueries{
// wasm
"/cosmwasm.wasm.v1/Query/ContractInfo": &wasmtypes.QueryContractInfoResponse{},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should be .v1.Query/ oops

@Reecepbcups
Copy link
Contributor Author

Going to have to postpone for a future upgrade, don't have time atm to write the test to ensure these all work.

@Reecepbcups Reecepbcups changed the title [feat!] Adds all stargate queries for smart contracts feat!: Adds all stargate queries for smart contracts Jul 18, 2023
@CosmosContracts CosmosContracts deleted a comment from elix1er Aug 8, 2023
@CosmosContracts CosmosContracts deleted a comment from elix1er Aug 8, 2023
@dimiandre
Copy link
Member

  • I do not think we have to care about the return values? Just that the query does not crash the chain

@Reecepbcups return values must be deterministic and in same order if array, same for return events

@Reecepbcups Reecepbcups added the blocked We can't do this until something else is done label Sep 19, 2023
@faddat
Copy link
Contributor

faddat commented Mar 1, 2024

@dimiandre is right, this is something we should be hyped and scared about, it needs tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked We can't do this until something else is done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On the necessity of AcceptedStargateQueries in Juno
5 participants