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

x/authz charge extra gas for GrantAuthorization #8595

Closed
wants to merge 13 commits into from

Conversation

aleem1314
Copy link
Contributor

Description

closes: #8311


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 16, 2021

Codecov Report

Merging #8595 (4aced77) into master (929b62c) will decrease coverage by 0.00%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8595      +/-   ##
==========================================
- Coverage   61.37%   61.37%   -0.01%     
==========================================
  Files         670      670              
  Lines       38286    38290       +4     
==========================================
+ Hits        23499    23501       +2     
- Misses      12329    12330       +1     
- Partials     2458     2459       +1     
Impacted Files Coverage Δ
x/auth/ante/sigverify.go 74.41% <ø> (ø)
x/authz/types/msgs.go 53.84% <ø> (ø)
x/auth/ante/basic.go 76.38% <60.00%> (-1.56%) ⬇️

@aleem1314 aleem1314 changed the title x/authz charge minimum gas-fee x/authz charge extra gas for GrantAuthorization Feb 24, 2021
@aleem1314 aleem1314 marked this pull request as ready for review February 25, 2021 12:07
txLen := len(ctx.TxBytes())
for _, msg := range tx.GetMsgs() {
if msg.Type() == authz.TypeMsgGrantAuthorization {
extraGasCost += params.TxSizeCostPerByte * uint64(txLen/len(tx.GetMsgs()))
Copy link
Member

Choose a reason for hiding this comment

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

How will this be known client side? Unless a user simulates the transaction it will fail. Out of scope of this PR but we should provide an endpoint that displays all the custom gas amount.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree with @marbar3778 here, this is way too hacky and not scalable

maybe tomorrow's discussion will give more clarity on an overall protocol base fee to avoid msg pollution, and we don't even need this PR anymore?

@@ -9,6 +9,7 @@ import (
"github.com/cosmos/cosmos-sdk/types/tx/signing"
"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
authz "github.com/cosmos/cosmos-sdk/x/authz/exported"
Copy link
Member

Choose a reason for hiding this comment

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

should auth depend on authz like this? I thought we were trying to avoid direct module dependencies.

@tac0turtle
Copy link
Member

Wont block this PR but dont think this should be merged. It seems too hacky to me.

txLen := len(ctx.TxBytes())
for _, msg := range tx.GetMsgs() {
if msg.Type() == authz.TypeMsgGrantAuthorization {
extraGasCost += params.TxSizeCostPerByte * uint64(txLen/len(tx.GetMsgs()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree with @marbar3778 here, this is way too hacky and not scalable

maybe tomorrow's discussion will give more clarity on an overall protocol base fee to avoid msg pollution, and we don't even need this PR anymore?

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.

Apologies for my delay here. Unfortunately this wasn't really the idea of the issue. We want to charge extra gas when creating an Authorization grant, not when running messages when they pass through the ante handler...

@anilcse
Copy link
Collaborator

anilcse commented Mar 3, 2021

Apologies for my delay here. Unfortunately this wasn't really the idea of the issue. We want to charge extra gas when creating an Authorization grant, not when running messages when they pass through the ante handler...

This is charging extra gas on creating an authorization and not while on executing the messages. Cc @aleem1314

@aaronc
Copy link
Member

aaronc commented Mar 3, 2021

Yes so basically this needs to be done in the authz MsgServer using ConsumeGas.

@aleem1314 aleem1314 marked this pull request as draft March 4, 2021 05:31
@clevinson
Copy link
Contributor

From our call on Wednesdsay, my memory is that we shouldn't add a gas based approach, but pursue a strategy that involves deposits that can be later retreived.

@clevinson clevinson closed this Mar 5, 2021
@alexanderbez alexanderbez deleted the aleem/8311-authz-gas branch April 22, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x/authz pruning and gas
6 participants