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.

Sign up for GitHub

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

Jump to bottom

feegrant filtered msgs #8604

Merged
merged 64 commits into from
Apr 9, 2021
Merged

feegrant filtered msgs #8604

merged 64 commits into from
Apr 9, 2021

Conversation

atheeshp
Copy link
Contributor

@atheeshp atheeshp commented Feb 17, 2021 •
edited
Loading

Description

closes: #8563

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Feb 17, 2021 •
edited
Loading

Codecov Report

Merging #8604 (06a9433) into master (6e26ee9) will increase coverage by 0.03%.
The diff coverage is 29.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8604      +/-   ##
==========================================
+ Coverage   58.97%   59.00%   +0.03%     
==========================================
  Files         576      577       +1     
  Lines       32377    32481     +104     
==========================================
+ Hits        19095    19167      +72     
- Misses      11041    11052      +11     
- Partials     2241     2262      +21
Impacted Files Coverage Δ
x/distribution/types/msg.go 47.54% <ø> (ø)
x/feegrant/keeper/grpc_query.go 0.00% <0.00%> (ø)
x/feegrant/keeper/msg_server.go 6.89% <0.00%> (-0.52%) ⬇️
x/feegrant/simulation/operations.go 76.99% <0.00%> (-0.21%) ⬇️
x/feegrant/types/filtered_fee.go 0.00% <0.00%> (ø)
x/feegrant/types/genesis.go 14.28% <0.00%> (-3.90%) ⬇️
x/feegrant/types/msgs.go 0.00% <0.00%> (ø)
x/bank/types/send_authorization.go 61.11% <20.00%> (+61.11%) ⬆️
x/feegrant/genesis.go 46.42% <25.00%> (-8.12%) ⬇️
x/feegrant/types/grant.go 44.18% <30.00%> (-4.47%) ⬇️
... and 21 more

aaronc
aaronc reviewed Feb 25, 2021
View reviewed changes
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

FilteredFeeAllowance looks right. How about keep things simple and just pass []sdk.Msg into the allowance?

x/feegrant/types/fees.go Outdated Show resolved Hide resolved
proto/cosmos/feegrant/v1beta1/feegrant.proto
google.protobuf.Any allowance = 1 [(cosmos_proto.accepts_interface) = "FeeAllowanceI"];

// allowed_messages are the messages for which the grantee has the access.
repeated string allowed_messages = 2;
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense.

@atheeshp atheeshp requested a review from aaronc March 1, 2021 11:00
atheeshp and others added 10 commits March 1, 2021 16:41
@atheeshp atheeshp marked this pull request as ready for review March 3, 2021 17:25
This was referenced Mar 30, 2021
Closed
Closed
@clevinson
Copy link
Contributor

@robert-zaremba suggesting we should go with the same (10 gas / iteration) as in #8984

@clevinson clevinson requested a review from aaronc April 7, 2021 16:42
@aaronc aaronc dismissed their stale review April 7, 2021 16:43

Issues addressed

@atheeshp
Copy link
Contributor Author

atheeshp commented Apr 8, 2021

Updated gas cost as per the discussion, @robert-zaremba, @clevinson or @AmauryM can you confirm the changes and add automerge label if it is fine. Thanks!

amaury1093
amaury1093 approved these changes Apr 8, 2021
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Looks good, if we can avoid panics that'd be ideal

x/feegrant/types/filtered_fee.go
Comment on lines +94 to +97
func (a *AllowedMsgFeeAllowance) PrepareForExport(dumpTime time.Time, dumpHeight int64) FeeAllowanceI {
allowance, err := a.GetAllowance()
if err != nil {
panic("failed to get allowance")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the signature here to return an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AmauryM this is in exporting genesis, I think panic is fine here.

aleem1314
aleem1314 suggested changes Apr 9, 2021
View reviewed changes
Copy link
Contributor

@aleem1314 aleem1314 left a comment

Choose a reason for hiding this comment

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

Overall LGTM. One small question, Afaik we removed feegrant/ante/ante.go in #8682. Any reason this is introduced again?

@atheeshp
Copy link
Contributor Author

atheeshp commented Apr 9, 2021 •
edited
Loading

Overall LGTM. One small question, Afaik we removed feegrant/ante/ante.go in #8682. Any reason this is introduced again?

This PR a bit older than the #8682. earlier, ante file was present in feegrant itself and the changes made here. May be that's why it is showing the feegrant/ante/ante.go file is created in this diff.

@aaronc aaronc added C:x/feegrant A:automerge Automatically merge PR once all prerequisites pass. labels Apr 9, 2021
@mergify mergify bot merged commit 7a3a156 into master Apr 9, 2021
@mergify mergify bot deleted the atheesh/feegrant-filtered-msgs branch April 9, 2021 16:49
@technicallyty technicallyty mentioned this pull request Apr 15, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amaury1093 amaury1093 amaury1093 approved these changes

@blushi blushi blushi approved these changes

@cyberbono3 cyberbono3 cyberbono3 approved these changes

@aleem1314 aleem1314 aleem1314 approved these changes

@alessio alessio Awaiting requested review from alessio

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@anilcse anilcse Awaiting requested review from anilcse

@clevinson clevinson Awaiting requested review from clevinson

@aaronc aaronc Awaiting requested review from aaronc

@robert-zaremba robert-zaremba Awaiting requested review from robert-zaremba

Assignees

@amaury1093 amaury1093

@cyberbono3 cyberbono3

@technicallyty technicallyty

Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/feegrant
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filter fee allowances by Msg