cosmos-sdk icon indicating copy to clipboard operation
cosmos-sdk copied to clipboard

feat: Periodic limits for MsgSend messages authorized via authz

Open nicolaslara opened this issue 2 years ago • 7 comments

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)

nicolaslara avatar Apr 29 '22 12:04 nicolaslara

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.

nicolaslara avatar May 02 '22 15:05 nicolaslara

Codecov Report

Merging #11829 (85440d3) into main (7a31a28) will decrease coverage by 0.24%. The diff coverage is 79.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

Impacted file tree graph

@@            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

codecov[bot] avatar May 13 '22 13:05 codecov[bot]

@nicolaslara what is your use case for this? sorry for being slow on reviews and providing feedback

tac0turtle avatar Jun 22 '22 08:06 tac0turtle

@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.

nicolaslara avatar Jun 22 '22 11:06 nicolaslara

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 send Y denom every 1h, with a total maximum of Z denom. This authorization expires on date 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.

nicolaslara avatar Jun 23 '22 09:06 nicolaslara

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.

github-actions[bot] avatar Aug 09 '22 00:08 github-actions[bot]

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

nicolaslara avatar Aug 09 '22 08:08 nicolaslara

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.

github-actions[bot] avatar Sep 24 '22 00:09 github-actions[bot]