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

Add instantiate permission to CodeInfoResponse #836

Conversation

jhernandezb
Copy link
Contributor

closes #829

@jhernandezb jhernandezb requested a review from alpe as a code owner May 4, 2022 04:23
@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #836 (4dad9b2) into main (7b78b7e) will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #836      +/-   ##
==========================================
+ Coverage   59.63%   59.87%   +0.23%     
==========================================
  Files          52       52              
  Lines        5948     5951       +3     
==========================================
+ Hits         3547     3563      +16     
+ Misses       2139     2120      -19     
- Partials      262      268       +6     
Impacted Files Coverage Δ
x/wasm/keeper/legacy_querier.go 69.89% <100.00%> (+0.32%) ⬆️
x/wasm/keeper/querier.go 70.00% <100.00%> (+7.87%) ⬆️
x/wasm/keeper/keeper.go 87.74% <0.00%> (-0.36%) ⬇️

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

👍 Very nice! thanks for the contribution.

I was surprised to see no tests were touched nor failing. It looks like none is actually covering the full response object. As this is part of the public API we should ensure the expected data is returned.
The legacy querier is deprecated already but adding 2 tests for the grpc querier would be a great contribution and fit into the scope of this PR.
Please let me know if you have capacity otherwise I can merge the PR as it is.

@jhernandezb
Copy link
Contributor Author

jhernandezb commented May 4, 2022

yes I can add the tests actually I already had one in draft will complete them and push the update.

@jhernandezb jhernandezb force-pushed the jhernandezb/add-access-info-to-query-response branch from 40ce965 to bf5f328 Compare May 4, 2022 13:48
@jhernandezb jhernandezb requested a review from alpe May 4, 2022 14:15
@jhernandezb
Copy link
Contributor Author

@alpe added query tests

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

thanks for the tests. looks good, but I will let @alpe take the final look and merge

codeId uint64
accessConfig types.AccessConfig
}{
"everybody": {
Copy link
Member

Choose a reason for hiding this comment

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

lgtm

}
}

func TestQueryCodeInfoList(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice to cover this as well

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Well done and thanks for the additional tests! 💪
I commented some nits as I hope you can add more with your contributions in the future.

)
})

allCodesResponse = append(allCodesResponse, types.CodeInfoResponse{
Copy link
Contributor

Choose a reason for hiding this comment

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

When I am strict, then there is a little smell as the test cases are not independent.
When you import with a "cached store" , the modified state would not be persisted

ctx, _ := parentCtx.CacheContext()

return codeInfo
}

codes := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

We mostly use map based table tests. I have this macro for Goland:

	specs := map[string]struct{
        $START$
	}{
		"$SECOND$": {
		$THIRD$
		},
		
	}
	for name, spec := range specs {
		t.Run(name, func(t *testing.T) {
            $FORTH$
		})
	}

@alpe alpe merged commit cd3c9dd into CosmWasm:main May 5, 2022
@shanev shanev deleted the jhernandezb/add-access-info-to-query-response branch May 6, 2022 01:43
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.

Add AccessConfig to CodeInfo query response
3 participants