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

On chain fee quoting #3

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

On chain fee quoting #3

wants to merge 7 commits into from

Conversation

tkporter
Copy link

@tkporter tkporter commented Nov 11, 2022

Looking for feedback on everything! Some specific things I'd like for you to mull over:

  1. Opinions on naming
  2. Opinions on having 2 separate functions depending on the default ISM
  3. Opinions on using LZ token exchange rates / prices when we can
  4. Opinions on continuing to not send txs if the message reverts
  5. Opinions on not calculating a specific fee based off the message calldata byte length

HIP-3.md Outdated
bytes32 indexed messageId,
uint256 amount,
uint256 gas,
IsmType ismType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this complexity is not worth it. I'd advocate for a simpler interface where the user just specifies how much gas they want on the remote chain. They need to understand that for "handle" anyways, I'm not sure it's more work to incorporate the gas costs of message verification into that

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious how you feel about the Default vs non-default ISM distinction section down below then

I really want to avoid having users default to an ISM that's out of their control and then they're responsible for being aware of changes to the ISM

Maybe there could be a specific InterchainGasPaymaster that's for default ISM consumers, and a separate one that's for non-default ISM consumers?

Copy link
Author

@tkporter tkporter Nov 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. imagine we have a quorum of 5 today on the default multisig ISM, people hardcode gas costs based off this, and now our hands are tied in ever increasing that quorum

HIP-3.md Outdated
* ISM.verify gas, and gas required by the recipient's `handle` function.
* @param _refundAddress The address to refund any msg.value overpayment to.
*/
function payGasForNonDefaultIsm(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just payGas

HIP-3.md Outdated
bytes32 indexed messageId,
uint256 amount,
uint256 gas,
IsmType ismType
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this parameter neccessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as it's written, there's slightly different relayer behavior based off the ism type

HIP-3.md Outdated

The off-chain relayer will index `GasPayment` events and relate them to dispatched messages based off their message ID.

For the default ISM case, the relayer will create the process transaction with a gas limit that results in at least `_handleGas` gas able to be used by the recipient’s `handle` function. The total gas limit of the transaction can be found by the relayer estimating all other gas costs (i.e. intrinsic gas, calldata gas including the ISM metadata and message body, ISM.verify gas, and Mailbox overhead gas) and adding this to the `_handleGas`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the IGP will need to calculate all of this also?

Copy link
Author

@tkporter tkporter Nov 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah so the idea is to have that be read from storage

The motivation behind separating default ISM from not is that the default ISM can and will change frequently and users shouldn't need to take action based off that

Copy link
Member

@yorhodes yorhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaving stale comments for now, will give another pass

HIP-3.md Outdated Show resolved Hide resolved
HIP-3.md Outdated Show resolved Hide resolved
HIP-3.md Outdated

For a message to be processed, a transaction must be made to the destination chain. Most Hyperlane user’s don’t want to be responsible for the infrastructure or manual effort required in submitting this transaction themselves, and prefer for a general purpose relayer to do this on their behalf. To ensure a relayer is properly compensated for transaction fees and cannot be griefed, messages must pay the relayer.

Today, an application hoping to calculate the appropriate payment for a message does so entirely off-chain using the Hyperlane SDK. When dispatching the message on the origin chain, some origin chain native tokens are paid to the relayer by calling the payable function `InterchainGasPaymaster.payGasFor`. The relayer observes the `GasPayment` event emitted by that function, and builds a database of payments for each message. The relayer will then continually re-evaluate whether messages are eligible for processing, which occurs if the USD value of the total payment for a message in origin chain native tokens exceeds the USD value of the destination transaction fees. In other words, there’s a social contract between message senders and the relayer that the relayer will submit the process transaction to the destination if it has been properly compensated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is native tokens necessarily native fee currency eg ETH? or can relayers accept other fee currencies eg USDC

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah a relayer could accept whatever token they want. Though for now I like using native tokens for a few reasons:

  1. users already have native tokens to pay for gas
  2. limiting choices can sometimes be nicer for users - reduces the feeling of being overwhelmed by options
  3. less for us to build - don't need to worry about a lot of exchange rates or another payGasFor function
  4. Apps that have users pay for gas don't need to add the address gasToken & uint256 gasPayment params to their own functions, they can just make their functions payable

The off-chain gas payment estimation, while nice for keeping as much data and calculation off chain to limit costs, has a number of downsides:

- It can be unclear if a message being dispatched will end up being processed by the relayer at its destination.
- Origin/destination chain native token exchange rates or destination chain gas prices can change, causing the USD value of the payment to be insufficient by the time the relayer evaluates the message for processing. A top-up payment would then be required, which is a poor user experience.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be interesting to consider stable fee currencies

HIP-3.md Outdated Show resolved Hide resolved
HIP-3.md Outdated Show resolved Hide resolved
HIP-3.md Outdated Show resolved Hide resolved
HIP-3.md Outdated Show resolved Hide resolved
HIP-3.md Outdated Show resolved Hide resolved
HIP-3.md Outdated Show resolved Hide resolved

An IGP that quotes and requires on-chain payment for a total amount of gas that is to be used by the ISM and recipient `handle` would look like:

```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark as solidity

* @param _gas An amount of gas.
* @param _refundAddress The address to refund any overpayment to.
*/
function payForGas(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be another implementation of this that omits _gas and _refundAddress? For destination chains where on-chain fee quoting isn't happening

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure I understand correctly: this would be in a scenario where someone is using our IGP that does perform on-chain fee quoting, but they are calling payForGas with a domain that doesn't have any oraclized fees / gas prices?

If so, we could make sure that the on-chain-fee-quoting IGP doesn't charge anything for domains it doesn't know any info about? Or possibly a flat default fee that doesn't scale with gas amount.

Or if you mean a situation where someone wants to use their own IGP that doesn't have any on-chain fee quoting support, they could just not use the _gas and _refundAddress at all, like we did before https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/6046ce68ef32b986d4ed813ad656d05497911815/solidity/contracts/InterchainGasPaymaster.sol#L50

Maybe I'm misunderstanding what you mean though. But I'm more inclined to have a single function interface if possible to avoid confusion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be in a scenario where someone is using our IGP that does perform on-chain fee quoting, but they are calling payForGas with a domain that doesn't have any oraclized fees / gas prices?

Correct. Basically I don't want to add additional requirements for supporting a new chain. Relaying should be able to work in the absence of on-chain oraclization

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To underscore, making deploying Hyperlane to new chains as easy as possible is one of the most important things we can do, and it's important that the gas payment protocol aligns with that goal

}
```

Hyperlane users are likely to be confident in the amount of gas that their recipient's `handle` function will use on the destination, but they likely don't have the same confidence in the amount of gas they need for their ISM. Users may be using the default ISM which could change entirely, or their ISM may undergo parameter changes that could change the amount of gas required. To provide a better experience, ISM-specific IGPs can be provided that allow users to supply the amount of gas their recipient's `handle` function consumes, and the ISM-specific costs are read from storage and accurately updated by the IGP owner.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refers to an "ISM specific IGP" in the singular, but contains a dictionary that is supposed to represent many remote ISMs. Which is it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mm good point, this is confusing. I guess a better way of saying this is that an IGP like this would be specific to many remote ISMs that an application is using. So we would provide an IGP that would relate to the default ISMs on many remote chains, because this is what we expect most people to use as they'll likely just be using default ISMs

* @param amount The amount of native tokens paid.
* @param gas The amount of destination gas paid for.
*/
event GasPayment(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging that this event does not handle unknown chains gracefully

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would not charging anything for domains it doesn't know any info about or charging some sort of default fee satisfy this for you? as suggested in https://github.com/hyperlane-xyz/hips/pull/3/files#r1073804672

Hyperlane users are likely to be confident in the amount of gas that their recipient's `handle` function will use on the destination, but they likely don't have the same confidence in the amount of gas they need for their ISM. Users may be using the default ISM which could change entirely, or their ISM may undergo parameter changes that could change the amount of gas required. To provide a better experience, ISM-specific IGPs can be provided that allow users to supply the amount of gas their recipient's `handle` function consumes, and the ISM-specific costs are read from storage and accurately updated by the IGP owner.

```
contract IsmFeeQuotingIgp {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea that there just be one of these that represents default ISMs everywhere? Or do you expect everyone that deploys an ISM to also deploy and configure one of these?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd want to provide one that represents default ISMs everywhere. And permissionless deployment chains can choose to do the same. And then if someone deploys their own non-default ISM, they can choose to deploy and configure one of these or skip it entirely and go with your suggestion of requiring applications to be aware of the ISM costs themselves. I don't think we need to be prescriptive with it when it comes to non-default ISMs


#### ISM-specific fee quoting IGPs

On the origin chain, an IGP cannot know what the length of an arbitrary ISM's metadata calldata is, nor can it know the expected gas used by an arbitrary ISM's `verify` function. Therefore, the IGP must get this information from some source.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IGP cannot, but the application sending the message can


Hyperlane users sending messages that use the default ISM should not need to worry about the destination gas costs incurred by the default ISM. This includes the metadata calldata and the `verify` function. Because the default ISM is out of user control and the parameters of the default ISM may change, a straightforward “happy path” for default ISM users is provided via the default ISM's IGP.

Any non-default ISMs may also have their own ISM-specific IGPs created. Alternatively, users can always call the fee quoting IGP directly with their own `_ismAndHandleGas` amount.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any non-default ISMs may also have their own ISM-specific IGPs created

I still strongly prefer that applications just include Hyperlane overhead in the amount of gas that they pay for

Copy link
Author

@tkporter tkporter Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By Hyperlane overhead you mean ISM costs right? Fwiw, they can always opt in to doing that by calling the fee quoting IGP directly.

Imo so much of the value of smart contracts is that you're generally able to just deploy something and it just "works" without worrying about operations / things no longer working. Most apps will just use the default ISM (at least for now), and if that default ISM ever changes, app contracts shouldn't be expected to make any changes on their side. Otherwise every time we want to change the quorum threshold, we risk breaking contracts because they could be specifying too low of a gas amount. If we tell people to use a generous buffer of like ~200k and say that the default ISM costs will never exceed that, it still feels like we're constraining what ISMs can do unnecessarily, while overcharging users.

I guess I see this whole ISM-specific IGP thing as providing the simplest experience for the majority of users while giving us the most flexibility for future changes without disruption. I'm not sure I understand the benefits of applications including the Hyperlane/ISM overhead themselves?

Copy link
Author

@tkporter tkporter Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fwiw, I feel most strongly about this for the default ISM case where people don't want to have to think about ISMs at all and just want things to work. For more complicated use cases like custom ISMs etc that people explicitly opt into, I feel less strongly. In those situations we can tell them to just call the fee quoting IGP directly and not deal with these gas-overhead-imposing-IGPs

function destinationGas(uint32 _destinationGas) external returns (uint256);
```

However, this conflicts with permissionless deployment of Hyperlane onto long-tail chains. A relayer can decide to offer its relaying services to process messages to long tail chains if they so choose, and it's more natural to put the onus on the relayer to specify what those costs are.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it's more natural to put the onus on the relayer to specify what those costs are.

Sorry, I'm not sure I follow this? Feels important, can you explaiN?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll clarify this in the HIP

I think this is trying to say that because a relayer decides which chains it wants to relay to, it's natural to have the relayer be responsible for maintaining an IGP that has the default ISM gas amounts for the remote chains it supports. And that putting the remote gas amounts inside an ISM doesn't make sense


The gas used by the message's bytes in the calldata of a process transaction are not explicitly calculated by the InterchainGasPaymaster because the upper limit of the gas cost is low, and performing this calculation on-chain introduces additional storage reads and computational complexity. Additionally, providing the InterchainGasPaymaster with the exact size in bytes of the Hyperlane message isn't straightforward.

#### Flexibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like to see a token amount only version of the interface, which IMO feels the most flexible

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering how you feel about just saying that people can implement and use their own IGP with the same function interface but they could just not care about gas amount / refund address / any on-chain fee quoting like we did before https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/6046ce68ef32b986d4ed813ad656d05497911815/solidity/contracts/InterchainGasPaymaster.sol#L50

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

Successfully merging this pull request may close these issues.

4 participants