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

Bank OCapKeeper #12404

Closed
4 tasks
robert-zaremba opened this issue Jun 30, 2022 · 8 comments
Closed
4 tasks

Bank OCapKeeper #12404

robert-zaremba opened this issue Jun 30, 2022 · 8 comments
Labels
C:Types C:x/bank S:needs architecture Needs a architecture proposal for how the feature will be implemented

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Jun 30, 2022

Summary

  • Add OCap model for x/bank.
  • Do not export existing Keeper out of x/bank

Problem Definition

Today, user doesn't have a way to allow spending limit to message handlers which will use x/bank keeper.
For example, today a keeper can depend on x/bank keeper and use user funds.
One way to work around this is to add additional parameter to the message, eg MaxFee: Coin. This way user can specify the maximum amount he is willing to pay when the message is processed.
However, the x/bank doesn't have any way to protect miss using the spend limits or allowance.

Proposal

DRAFT

  • Add protocol level x/bank authorization through a new keeper: OcapKeeper
  • Hide existing keeper in the x/bank/internal/keeper package

x/bank will can define OCap structure, which has to be pass to every x/bank execution. For example, in x/bank proto we can define:

message Allowance {
  repeated Coin coins = 1;
  // when empty, then we allow any destination 
  Address destination = 2;
  // optional, need to think how this can be defined and controlled.
  Address spender = 3;
}

Then we need to create a x/bank external keeper, which takes the Allowance parameter to every external method which mutates the state, eg:

type OcapKeeper interface {
   SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins, allowance Allowance) error
   ...
}

Then module, who want's do depend on bank keeper, will get standard interface, and the transaction middleware can inject the Allowance objects.

This can be generalized to every module.

Open problems:

  • how to make sure that Allowance is properly injected in OCapKeeper functions?

Related to


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added C:Types C:x/bank S:needs architecture review To discuss on the next architecture review call to come to alignment T: Architecture labels Jun 30, 2022
@alexanderbez
Copy link
Contributor

OCap isn't a way to address this IMO. I'm not really sure what you're trying to achieve here TBH. x/bank only sends from pre-defined accounts, either a user's account when executing a MsgSend or a module account when performing internal business logic. So what exactly is the issue you're trying to solve?

@robert-zaremba
Copy link
Collaborator Author

as an author of x/mymodule I can use bank keeper and call:

k.bankKeeper.SendCoins(ctx, fromAnyAccount, toAnyAccount, anyCoins)

and the bank keeper won't stop me from doing that.

What I want is that user will have to define Allowance and pair it with the transaction / message to inject into the bankKeeper methods.

@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Jun 30, 2022

Added a note about the middlewares, and injecting that ocap required objects into the wrappers:

  • x/mymodule depends on bank
  • in app wiring, whenever x/mymodule RPC is called, we provide a new instance of bank keeper: it will wrap OCapKeeper and set the Allowance based on the user provided Allowance.

One idea is that we can have a special messages: OcapMessages, which are not part of the transaction flow, but support messages which are going to handled by middlewares and injected into the "dynamic" dependencies (so here, bank Keeper is a dynamic dependency, instantiated whenever we process a message)

@robert-zaremba robert-zaremba added S:needs architecture Needs a architecture proposal for how the feature will be implemented and removed S:needs architecture review To discuss on the next architecture review call to come to alignment labels Jun 30, 2022
@aaronc
Copy link
Member

aaronc commented Jun 30, 2022

So you basically want a wrapper around a keeper? Sounds similar to what @fdymylja proposed with admission handlers, no? We can set something up like that with depinject, either using module-scoped providers or interface bindings.

@robert-zaremba
Copy link
Collaborator Author

@aaronc , it's a bit different. @fdymylja design is based on controllers (pre / post admission controllers) which work as pre /post ante handlers. They don't have a knowledge how one module will call another module.

The idea I'm proposing here will have the following flow:

  • bank exports OcapKeeper, where every method takes an extra argument: Allowance (we will still need to think about the final structure).
  • Moreover we will have a bank.KeeperBuilder which will take OCapKeeper and Allowance instances and return wrapper (which will pass allowance object into OCapKeeper) with standard `bank.Keeper.
  • Whenever a RPC A of a module M which depends on bank is called, an app middleware will update M.Keeper.Bank dependency (ideally through some trigger) with new OCapKeeper instance bind to the Allowance message. A will use bank keeper as it is using today.

So on every RPC call, module M will get updated dependencies.

That's one idea how this can be implemented. Maybe we can find other ways how this should work. The goal is that module M doesn't have a way to hack the Allowance message.

@alexanderbez
Copy link
Contributor

IMO I think this is complete overkill.

@robert-zaremba
Copy link
Collaborator Author

I think we need a standard for admissions between the modules.

@tac0turtle
Copy link
Member

closing in favour of #17579

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Types C:x/bank S:needs architecture Needs a architecture proposal for how the feature will be implemented
Projects
None yet
Development

No branches or pull requests

5 participants
@aaronc @robert-zaremba @alexanderbez @tac0turtle and others