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

Allow Relayers to overwrite payee address #1477

Closed
AdityaSripal opened this issue Jun 1, 2022 · 10 comments · Fixed by #1526
Closed

Allow Relayers to overwrite payee address #1477

AdityaSripal opened this issue Jun 1, 2022 · 10 comments · Fixed by #1526
Assignees
Labels

Comments

@AdityaSripal
Copy link
Member

AdityaSripal commented Jun 1, 2022

crypto dot com has a usecase involving paying the fees for relayer incentivization into a module account and then doing distribution from there. This can be supported if we have some method on the incentivizing chain that allows the payee address to be overwritten, so that we pay relayer fees to that address instead of the address we get from msg.sender or forward_relayer in the ack.

^This was based on an incorrect understanding.

The intended use case is that relayers can set a different payout address for ack/timeout that is different than msg.Sender.

We can either release this with 1.0 or add as a non-breaking feature addition

cc: @crodriguezvega @damiannolan

@damiannolan
Copy link
Member

Hey @AdityaSripal, so from my understanding of what you've written here is this a valid proposal:

  • Add a new a grpc endpoint RegisterDistributionAddress. This should be used on the incentivizing chain as you say, i.e. the packet source chain.
  • When a packet is ack'ed and the OnAcknowledgementPacket callback is fired within 29-fee we should lookup the distribution address stored in state.
  • If a distribution address is found then we should use it in place of ack.ForwardRelayerAddress and relayer args here
  • Similarly this should be done for packets in the OnTimeoutPacket callback and distribution address should be used in place of the relayer arg here
  • If a distribution address is not found in state then the current behaviour should be maintained.

How does this sound to you? Am I following correctly? Thanks!

@AdityaSripal
Copy link
Member Author

Add a new a grpc endpoint RegisterDistributionAddress

How would this be authenticated? From my understanding this is a proposal for a chain to route all packet fees to a different address. It also seems like the usecase in mind, it seems like this is just a chain-wide parameter. i.e. relayers do not have a choice on whether their funds get routed to this address or default behavior is chosen.

From my understanding everything would be correct in your proposal but the payee address should probably be a chain parameter rather than a grpc endpoint that anyone can call/is optionally called per relayer.

If parameter is set, then route all payments to the address in the parameter

If the parameter is not set, then do default behavior.

We should definitely double check with crypto.com to see which proposal fits their usecase and why they need it.

cc: @crodriguezvega

@damiannolan
Copy link
Member

From my understanding this is a proposal for a chain to route all packet fees to a different address.

Okay yeah, on second reading this makes more sense with your suggestion of an on-chain param. I misinterpreted when trying to understand the initial issue and great point, enforcing any kind of level of authentication of an endpoint like that wouldn't make sense.

Thanks for the info! I think the on-chain perhaps makes sense and gives me enough to make a start on the implementation.
Maybe we can follow up async with the crypto.com folks to ensure it fits their needs, but it sounds about right to me! :)

@allthatjazzleo
Copy link
Contributor

allthatjazzleo commented Jun 2, 2022

Hi ibc-go team,

When reading the current implementation of ics-029, we found that the recipient of incentives fees(ack-fee and timeout-fee) on source chain is the relayer address. However, it is possible for relayer to register arbitrary address of source chain on destination chain to receive the recv-fee.

Currently, for most of relayer applications like hermes only supports plain text hot wallet. It is not safe to top up too much token on relayer account in case of private key leakage. It is also inconvenient to top up multiple relayer accounts(one may have different relayer accounts for different channel or same channel for redundancy when deal with high ibc traffic).
Fortunately, the feegrant module comes out and is integrated into most of cosmos-sdk chains. Hermes also supports feegrant in its config. Fee granter account can fee grant relayer accounts and pay the actual relaying tx fee for relayer accounts. In this regard, relayer operator worries less about the breach or the balances of multiple relayer account because relayer operator only need to monitor and top up the fee granter account in this case. the relayer accounts should just hold minimal tokens.

Under ics-029, in source chain, we would like to seek the opportunity that if relayer account can optionally register their own destination address to receive the incentive fees(ack-fee and timeout-fee). If it is not registered, then the default recipient should be relayer address. It may be like the command but send to source chain?
In our case, we could register multiple relayer accounts to single fee granter account.

@AdityaSripal
Copy link
Member Author

Ahh ok, so your intention is not to have all funds go to a single address.

Instead you just want relayers to be able to specify a different address that funds should be sent to for Acks/Timeouts?

Yes this makes total sense to me and should be an easy addition.

As @damiannolan said, adding a GRPC endpoint like RegisterDistributionAddress that stores a mapping from the src relayer address and their preferred payout address will work.

Thanks for the suggestion!

@AdityaSripal
Copy link
Member Author

on second reading this makes more sense with your suggestion of an on-chain param. I misinterpreted when trying to understand the initial issue and great point, enforcing any kind of level of authentication of an endpoint like that wouldn't make sense.

No I think you had the right understanding and I was confused 😄

@AdityaSripal AdityaSripal changed the title Allow chain developers to overwrite payee address Allow Relayers to overwrite payee address Jun 2, 2022
@crodriguezvega
Copy link
Contributor

As @damiannolan said, adding a GRPC endpoint like RegisterDistributionAddress that stores a mapping from the src relayer address and their preferred payout address will work.

All right, then we agree to go for this solution, correct?

@AdityaSripal
Copy link
Member Author

Correct

@damiannolan
Copy link
Member

I've attempted to split up the work into more reasonable and easier to digest PRs. Please see:

With these we can proceed with the logic for overriding the fee payout in OnAcknowledgementPacket and OnTimeoutPacket callbacks.

Regarding the point made about authentication, and how do we authenticate such an endpoint if publicly exposed?
Should we consider gating the RegisterDistributionAddress rpc handler with an on-chain parameter? Essentially within RegisterDistributionAddress we can check an on-chain param and either return an error or proceed.

For example, something like the following would work:

feeDistribution: "default" | "custom"

customFeeDistributionEnabled: true | false

@AdityaSripal
Copy link
Member Author

Regarding the point made about authentication, and how do we authenticate such an endpoint if publicly exposed?

The endpoint is an SDK message, so we authenticate by verifying the message signer. We will have a message like this:

struct RegisterDistributionAddress{
     relayerAddress: string, // this is the address that will be submitting packet relay msgs (Ack/Timeout)
     payeeAddress: string // this is the address that will receive the funds
}

The expected signer must be the relayer address since it is the relayer who is saying I want you to pay this address. Note, if the signer was flipped then people could trivially appropriate funds by claiming that they were a particular relayer address.

func (msg RegisterDistributionAddress) GetSigners() []string {
    return []string{msg.relayerAddress}

ValidateBasic should check that both are valid SDK addresses (since they are both on the executing chain)

The message handler must set a mapping from the relayer address to the payee address.

func handler(msg RegisterDistributionAddress) {
     setPayeeAddress(msg.relayerAddress, msg.PayeeAddress)

Then in Ack and Timeout when we distribute the fee like so:

func distributeFee(relayerAddr string, funds sdk.Coin) {
     payeeAddr = getPayeeAddress(relayerAddr)
     // default to using the relayer address if separate payee address not set
     if payeeAddr == "" {
         payeeAddr = relayerAddr
     }
     
     send(payeeAddr, funds)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants