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

fix: stargate querier does not reset the structure state #1208

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Feb 23, 2023

Currently, the stargate querier marshal the result to the given structure stored in accepted list, but it does not reset the state. So the result will be unexpected. For example, the current stargate querier process is like:

cdc, _ := app.MakeCodecs()

protoResponse := &banktypes.QueryAllBalancesResponse{}

bz, err := cdc.Marshal(&banktypes.QueryAllBalancesResponse{
Balances: sdk.NewCoins(sdk.NewInt64Coin("test", 1)),
})

// First stargate query to QueryAllBalances 
err = cdc.Unmarshal(bz, protoResponse)
if err != nil {
panic(err)
}

// Second stargate query to QueryAllBalances 
err = cdc.Unmarshal(bz, protoResponse)
if err != nil {
panic(err)
}

fmt.Println(protoResponse.String())
// balances:<denom:"test" amount:"1" > balances:<denom:"test" amount:"1" >
// but the expected result is: balances:<denom:"test" amount:"1" >>

As a result, we should reset the protoResponse after marshaling.

@dadamu dadamu requested a review from alpe as a code owner February 23, 2023 10:44
@dadamu dadamu changed the title fix: stargate querier does not reset the state fix: stargate querier does not reset the structure state Feb 23, 2023
@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Merging #1208 (6d8018a) into main (88e01a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1208   +/-   ##
=======================================
  Coverage   57.49%   57.50%           
=======================================
  Files          54       54           
  Lines        7435     7436    +1     
=======================================
+ Hits         4275     4276    +1     
  Misses       2861     2861           
  Partials      299      299           
Impacted Files Coverage Δ
x/wasm/keeper/query_plugins.go 82.60% <100.00%> (+0.05%) ⬆️

@alpe
Copy link
Contributor

alpe commented Feb 24, 2023

Thanks for the PR and this important fix! I hope that we can resume OS work very soon. Please see #1198 for context

@alpe
Copy link
Contributor

alpe commented Feb 24, 2023

Would you mind to convert your description into a unit test for regression so that we also have confidence in the future?

@dadamu
Copy link
Contributor Author

dadamu commented Feb 24, 2023

@alpe Added.

@sonarcloud
Copy link

sonarcloud bot commented Feb 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@faddat
Copy link
Contributor

faddat commented Feb 25, 2023

Thanks!

This was referenced Feb 25, 2023
@alpe
Copy link
Contributor

alpe commented Mar 2, 2023

Great work! Thanks a lot for adding the test, too. Important work!🥇

@alpe alpe merged commit dc9a920 into CosmWasm:main Mar 2, 2023
alpe added a commit that referenced this pull request Mar 2, 2023
fix: stargate querier does not reset the structure state (backport #1208)
@dadamu dadamu deleted the paul/fix-stargate-querier branch March 2, 2023 17:03
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.

3 participants