-
Notifications
You must be signed in to change notification settings - Fork 625
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
Adding in a new Authorization Type for IBC Transfers in Authz #2431
Comments
Hi @jonathanjmak. Thanks for proposing this idea! I am trying to understand a bit how this would work, but I am not very familiar with I guess you would like to be able to use How would then this work if we want to avoid circular dependencies and shouldn't import ibc-go into the SDK? Because (in this example above using the CLI) eventually the SDK needs to instantiate an I think that it would be possible though to use directly the And regarding the CLI, I guess we could create a separate And regarding the optional How does this sound? Looking forward to your feedback! |
Hi @crodriguezvega, thanks for taking a look at this! I've addressed your points below. Hopefully they suffice and clear some things up (also open to feedback)
|
Thanks for your comments, @jonathanjmak! I was looking into this a bit more and trying to draft some code for discussion with the rest of the team. So based on the discussion here so far I think the work would involve the following:
syntax = "proto3";
package ibc.applications.transfer.v1;
option go_package = "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types";
import "cosmos/base/v1beta1/coin.proto";
import "gogoproto/gogo.proto";
import "cosmos_proto/cosmos.proto";
// TransferAuthorization allows the grantee to transfer up to spend_limit coins from
// the granter's account.
message TransferAuthorization {
option (cosmos_proto.implements_interface) = "Authorization";
repeated cosmos.base.v1beta1.Coin spend_limit = 1
[(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"];
// allow_list specifies an optional list of addresses to whom the grantee can transfer tokens on behalf of the
// granter. If omitted, any recipient is allowed.
repeated string allow_list = 2;
// source_port and source_channel specify the port ID and channel ID on which tokens can be transferred.
// If omitted, any combination of port ID and channel ID is allowed.
string source_port = 3;
string source_channel = 4;
} I realize that in my previous comment I made a mistake by suggesting to use the names
package types
import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/x/authz"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
)
// TODO: Revisit this once we have propoer gas fee framework.
// Tracking issues https://github.com/cosmos/cosmos-sdk/issues/9054, https://github.com/cosmos/cosmos-sdk/discussions/9072
const gasCostPerIteration = uint64(10)
var _ authz.Authorization = &TransferAuthorization{}
// NewSendAuthorization creates a new SendAuthorization object.
func NewTransferAuthorization(spendLimit sdk.Coins, allowedAddrs []string, sourcePort, sourceChannel string) *TransferAuthorization {
return &TransferAuthorization{
SpendLimit: spendLimit,
AllowList: allowedAddrs,
SourcePort: sourcePort,
SourceChannel: sourceChannel,
}
}
// MsgTypeURL implements Authorization.MsgTypeURL.
func (a TransferAuthorization) MsgTypeURL() string {
return sdk.MsgTypeURL(&MsgTransfer{})
}
// Accept implements Authorization.Accept.
func (a TransferAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authz.AcceptResponse, error) {
mTransfer, ok := msg.(*MsgTransfer)
if !ok {
return authz.AcceptResponse{}, sdkerrors.ErrInvalidType.Wrap("type mismatch")
}
toAddr := mTransfer.Receiver
limitLeft, isNegative := a.SpendLimit.SafeSub(sdk.NewCoins(mTransfer.Token)...)
if isNegative {
return authz.AcceptResponse{}, sdkerrors.ErrInsufficientFunds.Wrapf("requested amount is more than spend limit")
}
if limitLeft.IsZero() {
return authz.AcceptResponse{Accept: true, Delete: true}, nil
}
isAddrExists := false
allowedList := a.GetAllowList()
for _, addr := range allowedList {
ctx.GasMeter().ConsumeGas(gasCostPerIteration, "transfer authorization")
if addr == toAddr {
isAddrExists = true
break
}
}
if len(allowedList) > 0 && !isAddrExists {
return authz.AcceptResponse{}, sdkerrors.ErrUnauthorized.Wrapf("not allowed to transfer to address %s", toAddr)
}
if a.SourcePort != "" && a.SourcePort != mTransfer.SourcePort && a.SourceChannel != "" && a.SourceChannel != mTransfer.SourceChannel {
return authz.AcceptResponse{}, sdkerrors.ErrUnauthorized.Wrapf("not allowed to transfer on port ID (%s) and channel ID (%s)", mTransfer.SourcePort, mTransfer.SourceChannel)
}
return authz.AcceptResponse{
Accept: true,
Delete: false,
Updated: &TransferAuthorization{
SpendLimit: limitLeft,
AllowList: allowedList,
SourcePort: a.SourcePort,
SourceChannel: a.SourceChannel,
},
}, nil
}
// ValidateBasic implements Authorization.ValidateBasic.
func (a TransferAuthorization) ValidateBasic() error {
if a.SpendLimit == nil {
return sdkerrors.ErrInvalidCoins.Wrap("spend limit cannot be nil")
}
if err := a.SpendLimit.Validate(); err != nil {
return sdkerrors.Wrap(err, "invalid spend limit")
}
found := make(map[string]bool, 0)
for i := 0; i < len(a.AllowList); i++ {
if found[a.AllowList[i]] {
return ErrDuplicateEntry
}
found[a.AllowList[i]] = true
}
if a.SourcePort != "" && a.SourceChannel != "" {
if err := host.PortIdentifierValidator(a.SourcePort); err != nil {
return sdkerrors.Wrap(err, "invalid source port ID")
}
if err := host.ChannelIdentifierValidator(a.SourceChannel); err != nil {
return sdkerrors.Wrap(err, "invalid source channel ID")
}
}
return nil
} A lot of this code is similar to
ErrDuplicateEntry = sdkerrors.Register(ModuleName, 10, "duplicate entry") I have NOT tested any of this code, so tests (both integration and probably e2e) need to be written to make sure that functionality is correct, but I hope this serves as a starting point for discussion to asses if this pseudo-implementation goes in the right direction. @jonathanjmak Let me discuss with the rest of the team and I will comment back here. Just a couple of more questions for now:
|
This looks really good @crodriguezvega ! Really appreciate you getting back to us so quickly on this. The initial code outline looks pretty solid, and nice thought to already add in the error reporting for duplicate account addresses. I also think renaming it back to Question answers below:
|
Hi @crodriguezvega , just wanted to follow up on this to see if the team had any additional thoughts! |
Hi @jonathanjmak. Sorry I didn't come back to you earlier... The team has been pretty busy and I think we're still going to need some time until we can discuss this issue. Unfortunately it might take a couple of weeks... I am very sorry, but I hope you can understand and give us some more time. |
Hi @crodriguezvega, no worries -- I understand! I'll give you another ping in a few weeks, but in the meantim let me know if there is anything I can do to assist you further on this discussion. |
@crodriguezvega Any update from the team? |
Hi @jz5003. Sorry for not replying earlier. We have asked the SDK team to review the proposal to make sure that it all looks good. I will try to get their approval as soon as possible, so that work can be started on this. Sorry again for the delay. Is there any timeline where you would like to have this in a release of ibc-go? |
hi @crodriguezvega , preferably by the end of the year would be great! We'd love to integrate this since we think it will add in a lot of value and help make protocol interactions a lot smoother. |
This PR has been opened to implement it! @jonathanjmak @jz5003 please take a look and leave any feedback you may have. The implementation in the PR is slightly different than what we discussed here (the authorization could be granted for one or multiple channels at the same time), but hopefully that would also work for you. |
Hi @crodriguezvega , thanks for keeping us updated. I took a look and left a few comments. Let me know what I can do to help push this to the finish line! |
Requirements for this feature:
@jonathanjmak Do you agree with the requirements listed above? We will pick up this work in January and start opening PRs against the feature branch. |
@jonathanjmak We have picked up this work now and we have realized that implementing this requirement
will add extra complexity that can cause bugs in the future. Therefore we would like to waive this requirement for now and revisit later on, once the feature is released, if it is still needed. Do you agree with this approach? |
@jonathanjmak We have completed the feature and it's available in this branch, in case you want to have a look and do any testing and/or early integration. We will release this in v7. |
Summary
This is the same issue as cosmos/cosmos-sdk#13416, but now placing this request in the ibc-go repository so as to not introduce any circular dependencies.
Currently in the Cosmos SDK, the main types of authorization are stake, send, and generic authorization. With stake and send, there is an allowlist for authorization. However, if you wanted to authorize an IBC transfer, you would need to use generic authorization, which exposes unnecessarily loose permissions (because there is no allowlist, theoretically the authorized entity could IBC transfer to any address). We want to add in an authorization for IBC transfers to specific addresses via the allowlist.
Problem Definition
You don't want to use generic authorization for tasks outside of stake and send. It would make sense to add in an authorization type for IBC transfers given that is one of the most common use cases of the ecosystem. One example where this would be useful is authorizing an entity to programmatically conduct an IBC transfer to your existing wallets. Currently, you need to manually IBC transfer assets across chains yourself, but having this would allow you to easily IBC transfer assets across chains from an entity's app without the need to expose unnecessary authorizations.
Proposal
IBCTransferAuthorization
AllowList
,SpendLimit
AllowList
would contain a list of addresses authorized to receive IBC transferred assetsSpendLimit
is the amount that is authorized for each IBC transfer assetPort
,Channel
You can make implementation almost identical to
SendAuthorization
except now you are also making a new authorization type for IBC Transfers.Very similar to this issue that was accepted: cosmos/cosmos-sdk#12609
PR closing the above issue: cosmos/cosmos-sdk#12648
For Admin Use
The text was updated successfully, but these errors were encountered: