cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.27k stars 3.63k forks source link

Authz Fee Capture Request #11583

Open ironmandefi opened 2 years ago

ironmandefi commented 2 years ago

Summary

Authz empowers Cosmos developers to build noncustodial solutions. However, the integration of Authz in Defi-like products today prevents protocol engineers from capturing fees and creating an elegant user experience for customers. Authz doesn't allow us to charge a performance fee for our service as authz dynamically can't read state nor does it store a type to charge the fee owned by a user that the delegator can claim. The solution we propose is to implement stateful checks inside an authz execution and incorporate a fee type that can store the fee amount that needs to be paid out after a specific duration.

Problem Definition

The inclusion of a FeeAuthorization type may be complex and introduce security concerns unaware during implementation. However, this is a risk majority of new implications have and requires thorough testing and safeguards.

Proposal

The idea is to capture a fee or tax for the authx grant.

type TaxedAuthorization struct {
    Authorization
    TaxAmount sdk.Coins
}

And then, inside the Accept method, simply transfer the TaxAmount from the Authorization.Granter to the Authorization.Grantee. The solution would be to implement the statefull checks inside the authz execution, have that released into a new Cosmos SDK version.

We worked on this solution with @RiccardoM . Love to hear other input on how to best implement a fee capture mechanism in Authz. The introduction of this improvement will increase the adoption of this module.


For Admin Use

tac0turtle commented 2 years ago

Would love to hear from @amaurym and @aaronc. I think this is a interesting case in which someone is building tooling around authz and potentially other modules as a business and would like to take a cut.

robert-zaremba commented 2 years ago

@ironmandefi so you want to charge extra fee for executing authz actions. You can do that by creating (and registering) a custom type which implements Authorization interface. Check https://github.com/cosmos/cosmos-sdk/blob/release/v0.46.x/x/bank/types/send_authorization.go#L26 for an example. Technically it's possible to do state changes in Accept, although there could be a cleaner solution if we extend the Authorize interface to allow some custom callback after accept.

ironmandefi commented 2 years ago

Ahh so you are recommending just to create a custom type instead of messing with state changes. If we were to add a custom type, does this still allow other chains to update the newest "version" easily so we can interact with the authz module on other chains?

RiccardoM commented 2 years ago

Technically it's possible to do state changes in Accept,

Even though this is true, it's very hard to do so. If you want to create an authorization type that performs state changes, you only have access to the Context instance inside the Accept method. You don't have access to any keeper so even transferring tokens from one user to another will be very hard to do (you have to perform all the checks by hand and deal with store keys). This is technically doable, but practically impossible as of now

although there could be a cleaner solution if we extend the Authorize interface to allow some custom callback after accept.

I believe a better option would be to allow keepers to register authorization types themselves. This way developers would have access to the keeper instance inside the Accept method and be able to perform any kind of operation they want

ironmandefi commented 2 years ago

Alright fascinating thank you so much for your feedback on this topic :)

ironmandefi commented 2 years ago

Hi, now that .45.6 is out, is there anyway this issue get re looked at please. I think that will be super beneficial :) Now that so many projects are beginning to use the authz module

robert-zaremba commented 2 years ago

If we were to add a custom type, does this still allow other chains to update the newest "version" easily so we can interact with the authz module on other chains?

This will be a consensus breaking change. So won't be backported to 0.45 BTW: You can use authz with Interchain Accounts to have an elegant mechanism for executing something on other chain.

alexanderbez commented 2 years ago

So what's the consensus we're leaning towards here? Seems like a custom type won't suffice right? Or will it? If not, should we explore the API and UX space around making Authorize stateful?

RiccardoM commented 2 years ago

So what's the consensus we're leaning towards here? Seems like a custom type won't suffice right? Or will it? If not, should we explore the API and UX space around making Authorize stateful?

Authorize should be made stateful to allow modules to handle each authorization in whichever way they prefer. Ideally I believe each module should be able to register a set of [Authorization, AuthorizationHandler] where AuthorizationHandler can be something like:

type AuthorizationHandler = func (ctx sdk.Context, authorization Authorization) error

Then, after Accepting the authorization, the associated handler should be called. This should be enough to make it a lot easier to have custom logic like the one described in this issue

alexanderbez commented 2 years ago

Yeah I like that proposal. I don't think it's terribly complex to extend what exists today. @RiccardoM would you willing to submit an ADR? If so, and upon acceptance, we can work on it together.

RiccardoM commented 2 years ago

Yeah I like that proposal. I don't think it's terribly complex to extend what exists today. @RiccardoM would you willing to submit an ADR? If so, and upon acceptance, we can work on it together.

Not sure I have the bandwidth to handle this right now honestly. It might take me some weeks/a month to be able to do this

alexanderbez commented 2 years ago

No worries. I think we can put this into a sprint as we have enough of a foundation to build off of 👍

As this isn't super high priority, we'd put this into a future sprint (cc @marbar3778)