cosmos-sdk
cosmos-sdk copied to clipboard
feat: Periodic limits for MsgSend messages authorized via authz
Description
The current implementation of send authorizations via authz only allows for a fixed limit up to the expiration date. This pull request contains a proposed implementation for allowing the same period parameters used in feegrant (--period
and --period-limit
).
Documentation is currently missing, but I can added if there is interest in merging this (or a version of this).
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.
I have...
- [x] included the correct type prefix in the PR title
- [ ] added
!
to the type prefix if API or client breaking change - [x] targeted the correct branch (see PR Targeting)
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for building modules
- [ ] included the necessary unit and integration tests
- [ ] added a changelog entry to
CHANGELOG.md
- [ ] included comments for documenting Go code
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.
I have...
- [ ] confirmed the correct type prefix in the PR title
- [ ] confirmed
!
in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Thanks for the feedback @amaurym. Any answers I have regarding the demand for these feature would mostly be speculation. I'm experimenting with different parts of cosmos and needed this feature, so I saw it as an opportunity to learn about the internals of the SDK and contribute.
We built authz and shipped the 2-3 most basic authorizations on purpose.
That's interesting. I'd argue that periodic sends, along with a whitelist of addresses to send to is basic enough that it deserves a shared implementation. I'm not sure how used these authorizations are in the community, but AFAICT authz in general is not widely used (which I would guess is due to a lack of tooling around it; like a way to use it from keplr).
I've also been playing around with the idea of extending authz to build fully generic authorizations (storing arbitrary partial messages so that any message that matches them is authorized). This is a larger undertaking (and could even be its own authorization/custody chain), so I wanted to get the ball rolling and discuss this to see if there's an interest.
Codecov Report
Merging #11829 (85440d3) into main (7a31a28) will decrease coverage by
0.24%
. The diff coverage is79.31%
.
:exclamation: Current head 85440d3 differs from pull request most recent head aa70522. Consider uploading reports for the commit aa70522 to get more accurate results
@@ Coverage Diff @@
## main #11829 +/- ##
==========================================
- Coverage 66.28% 66.04% -0.25%
==========================================
Files 690 670 -20
Lines 72282 70637 -1645
==========================================
- Hits 47910 46649 -1261
+ Misses 21665 21317 -348
+ Partials 2707 2671 -36
Impacted Files | Coverage Δ | |
---|---|---|
x/bank/types/periodic_send_authorization.go | 73.91% <73.91%> (ø) |
|
x/bank/types/codec.go | 100.00% <100.00%> (ø) |
|
x/upgrade/keeper/msg_server.go | 63.63% <0.00%> (-36.37%) |
:arrow_down: |
x/feegrant/key.go | 72.72% <0.00%> (-27.28%) |
:arrow_down: |
x/staking/keeper/keeper.go | 57.50% <0.00%> (-20.00%) |
:arrow_down: |
x/slashing/keeper/signing_info.go | 72.22% <0.00%> (-14.45%) |
:arrow_down: |
x/staking/keeper/alias_functions.go | 32.25% <0.00%> (-12.91%) |
:arrow_down: |
x/feegrant/keeper/keeper.go | 72.41% <0.00%> (-8.18%) |
:arrow_down: |
x/staking/keeper/hooks.go | 62.00% <0.00%> (-6.00%) |
:arrow_down: |
x/staking/keeper/validator.go | 83.50% <0.00%> (-4.22%) |
:arrow_down: |
... and 59 more |
@nicolaslara what is your use case for this? sorry for being slow on reviews and providing feedback
@marbar3778 No worries! My use case was allowing a bot to get paid (X denom per month) for managing funds on osmosis, but the general feature is just for making authorizations on send more useful. Recurrent payments is a very basic feature (and already exists for feegrant), so I figured the best place for it was directly on the SDK.
I'm not fully understanding the relationship between period_spend_limit and spend_limit and periods in general.
This closely mimics the handling of periods on feegrant
. Basically: if --period
and --period-limit
are specified, then this will allow the grantee to send
the coins specified in period-limit
every period
time (say: 3osmo every week). If those aren't specified, the grant behaves as it currently does.
Internally, this is done by keeping track of the last send to have been approved via the grant and checking the clock time against the time of that transaction. Periodic send grants still can have the same limits as regular send grants (global expire date and global limit; independent of the periods).
So for example, you can specify:
-
Addr X
can sendY denom
every1h
, with a total maximum ofZ denom
. This authorization expires ondate D
A general use case for this is to allow someone (i.e.: a bot) to take a payment regularly for services provided, but and take the auth away if they misbehave. With the current authz implementation, you'd have to either authorize the full amount for a long period of time and trust the provider, or keep manually renewing the authorization periodically. An example of a service that could use this is https://restake.app/
Another use case would be to have a wallet that can send a certain amount every week (similar to a bank's weekly limits) to use while away from your private keys.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Haven't had time to focus on this. But there's only a bit of cleanup missing. I'll try to get this done within 2 weeks
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.