-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 FeeAccount to StdTx and add fee-delegation module #4616
Add FeeAccount to StdTx and add fee-delegation module #4616
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4616 +/- ##
==========================================
- Coverage 53.62% 52.96% -0.66%
==========================================
Files 260 271 +11
Lines 16176 16449 +273
==========================================
+ Hits 8675 8713 +38
- Misses 6855 7084 +229
- Partials 646 652 +6 |
Codecov Report
@@ Coverage Diff @@
## master #4616 +/- ##
=========================================
Coverage ? 52.96%
=========================================
Files ? 271
Lines ? 16449
Branches ? 0
=========================================
Hits ? 8713
Misses ? 7084
Partials ? 652 |
39780f6
to
7f268ac
Compare
if allowance == nil { | ||
return false | ||
} | ||
allow, updated, delete := allowance.Accept(fee, ctx.BlockHeader()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must set updated
back into store with the same key or the changes do not get persisted to application store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That happens below on line 78 below actually. I'll add a few code comments to make that clear
x/auth/ante.go
Outdated
@@ -122,18 +142,21 @@ func NewAnteHandler(ak AccountKeeper, supplyKeeper types.SupplyKeeper, sigGasCon | |||
signerAccs[0] = ak.GetAccount(newCtx, signerAccs[0].GetAddress()) | |||
} | |||
|
|||
if len(stdTx.FeeAccount) != 0 { | |||
if err := feeAccount.SetSequence(feeAccount.GetSequence() + 1); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think its necessary to update Sequence
of FeeAccount. It didn't sign anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right!
Thanks for working on this! Is there a reason you decided to go with this approach of allowing Fee Grantees to deduct directly from the Fee Granter account as opposed to the following:
|
The disadvantage of transferring into an escrow account is that this needs to be done for every granter/grantee pair and that may not be economically optimal for a granter who is allocating say daily spend limits to a lot of grantees. A common scenario that I am imagining is that an organization wants to create a fee pool for their employees or a third party service wants to offer this for a fee (in some other currency), and that these grants would be on a monthly/daily/hourly spend limit basis. So a granter might want to over-extend their total max daily allocation to a pool given actual usage and only recharge as needed. If I give 100 people a 10 atom daily spend limit for a validity of say 30 days, I shouldn't need to give each user 300 atoms in escrow (a total of 30000 atoms). I can just have one account that has say 1000 atoms max at a time, monitor usage and fill it up as needed. Those 100 people may only spend 5000 atoms total over the course of a month, but if I did it as escrow upfront I might need to lockup 30000 atoms. So in my reasoning, letting granters pool funds into one account they fill up as needed is more economically practical for granters than escrow. Also the way I'm imagine delegation in general is that it's not so much of a promise/contract - like I promise you this much money so I'll put it in escrow to make sure you have it. But rather more like a privilege - you have this privilege to spend my money but if you abuse it, I can revoke it. |
@aaronc thanks for the PR! I'd be happy to review this and provide feedback, but do you mind first updating the PR to reflect the latest changes on |
… and CLI context Co-authored-by: Ethan Frey <[email protected]>
b9fa3bc
to
af9743a
Compare
Thanks @alexanderbez. Just rebased again. In my mind the most important thing to review currently are the changes to Also, I forgot to mention in my previous comment the sub-key use case. In this scenario a user would have a single account and would delegate fees to other "sub keys". So it's one account, just different keys. Other features to support sub-keys will be coming in 2 PR's that I am preparing. This has been discussed on telegram with others including B-harvest, and would likely supersede the previous sub-key proposal being more general to cover other cases like the organizational use case described above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work @aaronc -- it was a pleasure reviewing this (apologies for the delay). I'm in favor with the general strategy taken here. Few things to note apart from the comments I've already left:
- This needs a corresponding spec doc and also preferably a
doc.go
in the root of the module (although not necessary) - The new module should follow the module spec outlined here
🎉
@@ -32,10 +32,16 @@ func init() { | |||
// and also to accept or reject different types of PubKey's. This is where apps can define their own PubKey | |||
type SignatureVerificationGasConsumer = func(meter sdk.GasMeter, sig []byte, pubkey crypto.PubKey, params Params) sdk.Result | |||
|
|||
type FeeDelegationHandler interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a godoc to FeeDelegationHandler
?
@@ -32,10 +32,16 @@ func init() { | |||
// and also to accept or reject different types of PubKey's. This is where apps can define their own PubKey | |||
type SignatureVerificationGasConsumer = func(meter sdk.GasMeter, sig []byte, pubkey crypto.PubKey, params Params) sdk.Result | |||
|
|||
type FeeDelegationHandler interface { | |||
// AllowDelegatedFees checks if the grantee can use the granter's account to spend the specified fees, updating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we wrap the comments at 80 chars (if not already)?
type FeeDelegationHandler interface { | ||
// AllowDelegatedFees checks if the grantee can use the granter's account to spend the specified fees, updating | ||
// any fee allowance in accordance with the provided fees | ||
AllowDelegatedFees(ctx sdk.Context, grantee sdk.AccAddress, granter sdk.AccAddress, fee sdk.Coins) bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AllowDelegatedFees(ctx sdk.Context, grantee sdk.AccAddress, granter sdk.AccAddress, fee sdk.Coins) bool | |
AllowDelegatedFees(ctx sdk.Context, grantee, granter sdk.AccAddress, fee sdk.Coins) bool |
// any fee allowance in accordance with the provided fees | ||
AllowDelegatedFees(ctx sdk.Context, grantee sdk.AccAddress, granter sdk.AccAddress, fee sdk.Coins) bool | ||
} | ||
|
||
// NewAnteHandler returns an AnteHandler that checks and increments sequence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets update the godoc to now mention the use of the new fee delegation logic.
if err != nil { | ||
return err | ||
} | ||
txBldr.WithFeeAccount(feeAcccountAddr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gofmt
return err | ||
} | ||
|
||
fmt.Println(string(res)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use return cliCtx.PrintOutput(...)
instead here. That means we'll have to decode res
into the correct type.
func GetCmdDelegateFees(cdc *codec.Codec) *cobra.Command { | ||
return &cobra.Command{ | ||
Use: "delegate [grantee] [fee-allowance]", | ||
Short: "delegate", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the nomenclature perhaps be grant
instead?
|
||
// GenesisState defines genesis data for the module | ||
type GenesisState struct { | ||
// Delegations []Delegation `json:"delegations"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??? Is this still to be implemented?
Granter sdk.AccAddress `json:"granter"` | ||
} | ||
|
||
func (k Keeper) GetFeeAllowances(ctx sdk.Context, grantee sdk.AccAddress) []FeeAllowanceGrant { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern we've been following in the SDK is to instead provide/expose iterators with a cb function param instead. We can keep this method, but we should also have an iterator method which this would simply use.
if allowance == nil { | ||
return false | ||
} | ||
allow, updated, delete := allowance.Accept(fee, ctx.BlockHeader()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@aaronc are you planning to resume work here or can we close this PR? |
Yes l am. Thanks.
…On Wed, Sep 11, 2019, 04:28 Federico Kunze ***@***.***> wrote:
@aaronc <https://github.com/aaronc> are you planning to resume work here
or can we close this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4616?email_source=notifications&email_token=AAAL6FXMKJBQI337RYDPCCLQJCTZLA5CNFSM4H3CBWFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6NWMOQ#issuecomment-530277946>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAL6FWPVHOLBVDKCJ73REDQJCTZLANCNFSM4H3CBWFA>
.
|
Please remove the |
@aaronc I am happy to work on addressing PR comments and updating to new module spec here. However, can you add a high-level doc.go file explaining the use and main interfaces. I would much prefer to have a clear requirements than just assuming what is coded is correct. I guess the "proper" way now is doing an ADR, but at least some clear docs is good. |
…asicFeeAllowance and PeriodicFeeAllowance
@ethanfrey I have added a doc.go and also better doc comments on |
Replaced by #5207 |
Adds:
FeeAccount sdk.AccAddress
field toStdTx
AnteHandler
to deduct fees from theFeeAccount
if one is provided and that account has indeed delegated fees to the account signing the transactionfee-delegation
module to manage granting of fee allowances.FeeAllowance
is an interface which allows for different types of fee allowance policies such as daily or hourly limits--fee-account
flagUse cases:
This PR is squashes many commits from the Gaian's team at Hackatom Berlin. It is now being worked on in the context of the Community Group on Key Management (https://github.com/cosmos-cg-key-management) and Regen Network intends to test it in on its live testnet in the coming weeks. Once the testing is complete and the community has had a time to discuss the design more thoroughly, we intend to move this PR out of draft status. In the meantime, this thread can be a forum for discussion and review of the proposed changes.
docs/
)clog add [section] [stanza] [message]
Files changed
in the github PR explorerFor Admin Use: