feat(profitability): Remove hardcoded min revenue map and add logic to consider gas cost #212

kevinuma · 2022-07-15T15:59:52Z

No description provided.

kevinuma · 2022-07-15T16:00:47Z

src/clients/MultiCallerClient.ts

@@ -26,7 +26,7 @@ export interface AugmentedTransaction {
 export class MultiCallerClient {
  private transactions: AugmentedTransaction[] = [];
  // eslint-disable-next-line no-useless-constructor
-  constructor(readonly logger: winston.Logger, readonly gasEstimator: any, readonly maxTxWait: number = 180) {}


Unused vars

kevinuma · 2022-07-15T16:01:03Z

src/clients/ProfitClient.ts

@@ -6,7 +6,7 @@ import { Coingecko } from "@uma/sdk";
 // Define the minimum revenue, in USD, that a relay must yield in order to be considered "profitable". This is a short
 // term solution to enable us to avoid DOS relays that yield negative profits. In the future this should be updated
 // to actual factor in the cost of sending transactions on the associated target chains.
-const chainIdToMinRevenue = {
+const MIN_REVENUE_BY_CHAIN_ID = {


nit: Constants should be ALL_CAPS

nicholaspai · 2022-07-15T16:41:46Z

src/clients/ProfitClient.ts

+  288: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH
+  42161: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH
+};
+// TODO: Make this dynamic once we support chains with gas tokens that have different decimals.


I hope there is no world where a chain has a non-18 decimal gas token but sadly this will probably be the case

nicholaspai · 2022-07-15T16:42:03Z

src/clients/ProfitClient.ts

@@ -22,6 +22,18 @@ const chainIdToMinRevenue = {
  80001: toBNWei(1),
 };

+// We use wrapped ERC-20 versions instead of the native tokens such as ETH, MATIC for ease of computing prices.
+const WMATIC = "0x0d500B1d8E8eF31E21C99d1Db9A6444d3ADf1270";


nit: since you hardcode the WETH address below you might as well just hardcode this address in the below dict

I have this out so I can fetch coingecko price for it separately further below. The coingecko request needs to be separate since the platform_id passed in the request needs to be "polygon_pos", not "ethereum".

ohh sorry I missed this comment. Sounds good

kevinuma · 2022-07-16T18:41:56Z

src/clients/ProfitClient.ts

@@ -41,7 +54,7 @@ export class ProfitClient {

  getPriceOfToken(token: string) {
    if (!this.tokenPrices[token]) {
-      this.logger.warn({ at: "ProfitClient", message: `Token ${token} not found in state. Using 0` });
+      this.logger.debug({ at: "ProfitClient", message: `Token ${token} not found in state. Using 0` });


This can be noisy when coingecko apis keep failing

I'm good with this, coingecko API seems to fail a lot. Any ideas for a fallback strategy?

mrice32

This is awesome work! A few high-level comments.

mrice32 · 2022-07-17T20:39:36Z

src/clients/ProfitClient.ts

+
+    // Consider gas cost.
+    const gasCostInUsd = gasUsed
+      .mul(this.getPriceOfToken(GAS_TOKEN_BY_CHAIN_ID[deposit.destinationChainId]))


Don't we need to compute the current gas price to take into account fluctuations in relayer costs? I think most chains have gas costs that can vary by up to 10x week-to-week, so I don't think hardcoding USD revenue will be sufficient to match the dynamic estimations being done by the frontend.

Note: maybe I'm missing the point of this PR! Feel free to clarify in the description. I'm just going off of the title.

gasUsed already includes the gas fee (gasUsed = gas cost * gas fee) as passed in from Relayer.

Regarding the hardcoded min USD revenue. We'll likely remove them as the first step as we currently have a lot of small deposits with < $1 profit (after gas costs). Depending on external relayer demand, we might or might add back the ability to configure a min profit amount.

Hmmm, maybe we're just using different language to describe the same thing.

I'm talking about the USD costs, which would amount to gasUsed * gasPrice * gasTokenPrice. I don't think this code takes gasPrice into account, so if the gas price swings very high, we could relay unprofitable deposits, and if it swings low, we could block profitable deposits.

I think if we want this code to correctly take into account the true cost of relaying a transaction, we need to estimate the gas cost that will be paid.

Do you agree with that assessment?

That's what I was talking about. That multiplication you're looking for is done in https://github.com/across-protocol/relayer-v2/pull/212/files#diff-562d70c5c8a2a3e8074947c1f599ce3c57b2abd4cdff462422b5bc9fa4257e9fR76

Ah, i understand matt's confusion. Its that usually "gas used" in web3/ethers context refers the EVM gas assessed for a smart contract function. Also, estimateGas is a common function name that refers to the EVM gas cost of a function. Perhaps this would be reduce confusion if you renamed to estimateGasCost or estimateTotalCost wherever you call estimateGas. (I believe this is in the Relayer.ts file)

Yep, exactly. Sorry about that @kevinuma. You're completely right. Thanks for the clarification!

mrice32 · 2022-07-17T20:45:44Z

src/clients/ProfitClient.ts

    this.logger.debug({ at: "ProfitClient", message: "Updating Profit client", l1Tokens });
-    const prices = await Promise.allSettled(l1Tokens.map((l1Token: L1Token) => this.coingeckoPrice(l1Token.address)));
+    const priceFetches = l1Tokens.map((l1Token: L1Token) => this.coingeckoPrice(l1Token.address));


Have you considered trying to use the same SDK functionality as the FE to compute the expected relayer fee?

One failure mode that we saw in across v1 when we had separate implementations of the expected fee was that our frontend would produce transactions that our bot would then feel were underpriced or vice-versa. This isn't so bad if the bot's estimation is lower than the frontend, but when the frontend estimated lower than the bot, we would end up not relaying the transaction, creating a bad UX and support tickets where people ask when their money will arrive.

The strategy that we used was to use the same library to compute the estimated relayer fee in both places and get the FE to charge a configurable percentage more than the bot to ensure that small swings in gas prices between the FE estimation and the bot seeing the deposit wouldn't cause a deposit to get stuck in limbo.

Thoughts?

My next followup is to update the SDK. I think the SDK would need a slightly different implementation for including gas costs in the relayer fee. We currently have a lot of small deposits with single digit in cents of profits. So the SDK seems to be underpricing some deposits. We'll need to add a gas buffer amount in SDK in case of gas spikes.

I think the SDK would need a slightly different implementation for including gas costs in the relayer fee.

Why do you think the computation in the frontend should be different from the relayer? How do you think they should differ, specifically?

We currently have a lot of small deposits with single digit in cents of profits.

Is there anything wrong with profiting a few cents on a small, cheap transaction? For instance, if the gas is $0.001 and the capital required is $0.50, a profit of $0.005 seems pretty reasonable, no?

We'll need to add a gas buffer amount in SDK in case of gas spikes.

Agreed, a buffer percentage makes a lot of sense! Hopefully, this could just be a difference in parameterization of the same computation.

My next followup is to update the SDK.

Rather than reinventing the wheel, why not start with integrating the SDK implementation that's running in prod and takes into account gas prices and configurable capital costs and try to fit that into this implementation, fixing any bugs or incompatibilities we find along the way?

This has the added advantage of ensuring that the results of both computations will match up every step along the way.

I think the SDK would need a slightly different implementation for including gas costs in the relayer fee.

Why is this?

Why do you think the computation in the frontend should be different from the relayer? How do you think they should differ, specifically?

I think the bot should potentially have a different gas buffer from the SDK. For example when computing relayer fee %, the SDK can use a buffer of 20 gwei on L1 for gas fee buffer (let's say current gas is 20-30 gwei). The relayer, on the other hand, might want to use 30 gwei instead when computing gas cost in profitability, because it wants to absolutely be sure it'd be profitable and not incur any loss due to gas spikes

Is there anything wrong with profiting a few cents on a small, cheap transaction? For instance, if the gas is $0.001 and the capital required is $0.50, a profit of $0.005 seems pretty reasonable, no?

yes but this thin margin means there's a decent chance of incurring a loss if gas turns against you.

My next followup is to update the SDK.

Makes sense. Ideally both SDK and relayer should be using the same formula/calculation but potentially with different gas buffer. I'll update this PR to use the SDK's code.

I think the bot should potentially have a different gas buffer from the SDK. For example when computing relayer fee %, the SDK can use a buffer of 20 gwei on L1 for gas fee buffer (let's say current gas is 20-30 gwei). The relayer, on the other hand, might want to use 30 gwei instead when computing gas cost in profitability, because it wants to absolutely be sure it'd be profitable and not incur any loss due to gas spikes

Couldn't the relayer instantiate the SDK with a different gasBuffer input in this case? I agree with your example .

I think the bot should potentially have a different gas buffer from the SDK. For example when computing relayer fee %, the SDK can use a buffer of 20 gwei on L1 for gas fee buffer (let's say current gas is 20-30 gwei). The relayer, on the other hand, might want to use 30 gwei instead when computing gas cost in profitability, because it wants to absolutely be sure it'd be profitable and not incur any loss due to gas spikes

Couldn't the relayer instantiate the SDK with a different gasBuffer input in this case? I agree with your example .

@nicholaspai agree that this should be a parameterization of the SDK. Rather than it being a GWEI buffer, might be easier for us to make it percentage-based since gas prices work differently on different chains (for instance arbitrum and optimism have two types of gas), so a GWEI offset doesn't really work.

On your example @kevinuma, I think we'll ultimately want to make this configurable in both places as a GAS_MULTIPLIER, GAS_MARGIN, or GAS_DISCOUNT (same thing but inverted sign), so each caller can customize it. However, I think we probably want it to be the opposite of your example. In your example, my understanding is that the FE/API would always be underpriced relative to the relayer, so we'd be blocking most relays due to unprofitability.

I think we want the UI to be more conservative than the relayer, because we want to avoid users who use our UI or API getting stuck (this is a bad product UX and creates support tickets). So I think, in general, we want GAS_MARGIN in the frontend/api to be greater than the default GAS_MARGIN in the relayer. Note: we can't control how 3rd parties set this, but we'd want them to use the default so they are willing to relay 99.9% of deposits sent through the FE/API.

Does that make sense?

Is there anything wrong with profiting a few cents on a small, cheap transaction? For instance, if the gas is $0.001 and the capital required is $0.50, a profit of $0.005 seems pretty reasonable, no?

yes but this thin margin means there's a decent chance of incurring a loss if gas turns against you.

I'm splitting hairs here a bit, but just because the absolute size and costs are low doesn't mean we have a higher chance of losing money I don't think. Likelihood of loss is dependent on gas price volatility. The amount of money lost on a particular transaction is likely to be lower on a chain that consistently has lower gas prices. For instance, a bot that had no profitability module lost > $1k on a single transaction in Across v1 due to a mainnet gas spike. This size of loss is nearly impossible on a cheap chain, like polygon because gas prices just never get high enough.

Either way, I think we're aligned on how this system should work!

kevinuma · 2022-07-18T00:49:54Z

src/clients/ProfitClient.ts

-// Define the minimum revenue, in USD, that a relay must yield in order to be considered "profitable". This is a short
-// term solution to enable us to avoid DOS relays that yield negative profits. In the future this should be updated
-// to actual factor in the cost of sending transactions on the associated target chains.
-const chainIdToMinRevenue = {


Context: https://risklabs.slack.com/archives/D03GKFYCDJ7/p1657999254013629. tldr; is this hardcoded limit is filtering out a lot of small deposits with profits of < $1. When running with this previous logic, my relayer was skipping 9/10 deposits. We'll follow up with data analysis to understand profitability of small deposits.

nicholaspai

Nice PR left some comments/questions. Definitely think we should try to re use the sdk-v2 if possible!

src/clients/ProfitClient.ts

nicholaspai · 2022-07-18T01:23:41Z

src/utils/TransactionUtils.ts

+    const args = transaction.value ? [...transaction.args, { value: transaction.value }] : transaction.args;
+    const [gasUsed, gasFee] = await Promise.all([
+      transaction.contract.estimateGas[transaction.method](...args),
+      transaction.contract.provider.getGasPrice(),


Can you get this gas price once per relayer loop and then re use it for all invocations of estimateGas? It seems like this would be a low risk way to reduce requests to the RPC.

Also, is provider.getGasPrice the same method that is used by runTransaction to get gas price? Do we not use a GasEstimatorClient? I only ask because we should be consistent with how we query gas price

Also I just realized, why don't you use utils/TransactionUtils/getGasPrice here? I've never used provider.getGasPrice before so I'm not sure if it does what you think it does

It basically gives you the result of eth_gasPrice from the node. The rough algorithm used to compute this is here. I think this is pretty reasonable. I think the SDK does something similar for some chains.

The issue I see with this approach is that I'm not sure it will give reasonable results for all L2s. Optimism has a custom provider that has a special method to compute the estimated gas costs because optimism has multiple types of gas (see the SDK implementation).

For the other chains, this approach should mostly work I think.

We mostly follow these rules in the SDK, specific to each chain, but there are definitely areas to improve the estimation.

Regardless of how eth_gasPrice works shouldn't we be consistent and use our own implementation of getGasPrice.ts?

nicholaspai

Left some more comments:

feat(profitability): Remove hardcoded min revenue map and add logic to consider gas cost #212 (comment)
feat(profitability): Remove hardcoded min revenue map and add logic to consider gas cost #212 (comment)

mrice32

This looks great! A few comments, but I think this mostly works as intended.

mrice32 · 2022-07-19T20:44:22Z

src/utils/TransactionUtils.ts

+    const args = transaction.value ? [...transaction.args, { value: transaction.value }] : transaction.args;
+    const [gasUsed, gasFee] = await Promise.all([
+      transaction.contract.estimateGas[transaction.method](...args),
+      transaction.contract.provider.getGasPrice(),


It basically gives you the result of eth_gasPrice from the node. The rough algorithm used to compute this is here. I think this is pretty reasonable. I think the SDK does something similar for some chains.

The issue I see with this approach is that I'm not sure it will give reasonable results for all L2s. Optimism has a custom provider that has a special method to compute the estimated gas costs because optimism has multiple types of gas (see the SDK implementation).

For the other chains, this approach should mostly work I think.

We mostly follow these rules in the SDK, specific to each chain, but there are definitely areas to improve the estimation.

mrice32 · 2022-07-19T20:46:43Z

src/utils/TransactionUtils.ts

+  try {
+    const args = transaction.value ? [...transaction.args, { value: transaction.value }] : transaction.args;
+    const [gasUsed, gasFee] = await Promise.all([
+      transaction.contract.estimateGas[transaction.method](...args),


One thing we need to be careful about when estimating gas on this transaction is that it will revert if the estimation is sent from a wallet that either doesn't have enough of the currency or has not approved the contract to pull that token. So we need to make sure that this defaults to using a wallet that will always work.

Is it possible to get here if the relayer doesn't have the balance to run this transaction?

Is the relayer address always used as the default from address in this simulated transaction?

To be safe we can use the address hardcoded here in the sdk-v2 repo

mrice32 · 2022-07-19T20:48:12Z

src/utils/TransactionUtils.ts

@@ -63,6 +63,23 @@ export async function willSucceed(
  }
 }

+export async function estimateGas(


nit: I think it might be clearer if we called this estimateGasCost, since estimateGas is the name of the method that is usually used to report the amount of gas rather than the amount of tokens used for gas.

nicholaspai · 2022-08-03T13:46:22Z

Please add a PR description @kevinuma !

kevinuma added 2 commits July 15, 2022 08:19

Test

7198daa

Fix

9395e48

kevinuma requested review from mrice32, nicholaspai and chrismaree as code owners July 15, 2022 15:59

kevinuma marked this pull request as draft July 15, 2022 15:59

kevinuma commented Jul 15, 2022

View reviewed changes

kevinuma added 2 commits July 15, 2022 09:32

Fix

a9b0ab4

Commit

e3233ad

nicholaspai reviewed Jul 15, 2022

View reviewed changes

Commit

ed9d47f

nicholaspai reviewed Jul 15, 2022

View reviewed changes

kevinuma added 5 commits July 15, 2022 09:51

Commit

fe1f6fd

Commit

a9e9c30

Commit

e52a467

Commit

cd44aae

Add tests

2e7b298

kevinuma marked this pull request as ready for review July 16, 2022 18:07

kevinuma added 2 commits July 16, 2022 11:11

nit

ca839bd

fix

d232b02

kevinuma requested a review from nicholaspai July 16, 2022 18:41

kevinuma commented Jul 16, 2022

View reviewed changes

mrice32 reviewed Jul 17, 2022

View reviewed changes

kevinuma added 2 commits July 17, 2022 17:21

remove hard coded min revenue

e850af8

fix tests

0d94335

kevinuma requested a review from mrice32 July 18, 2022 00:47

kevinuma changed the title ~~feat: Update Profitability logic to consider gas cost~~ feat(profitability): Remove hardcoded min revenue map and add logic to consider gas cost Jul 18, 2022

kevinuma commented Jul 18, 2022

View reviewed changes

nicholaspai reviewed Jul 18, 2022

View reviewed changes

nicholaspai reviewed Jul 19, 2022

View reviewed changes

mrice32 reviewed Jul 19, 2022

View reviewed changes

james-a-morris mentioned this pull request Aug 8, 2022

improve: update sdk queries to accurately estimate gas costs across-protocol/sdk#64

Merged

kevinuma marked this pull request as draft August 13, 2022 19:24

kevinuma closed this Aug 16, 2022

kevinuma mentioned this pull request Aug 18, 2022

feat(relayer): Consider gas cost when calculating profitability #261

Merged

pxrl deleted the gasprofit branch September 5, 2024 10:30

feat(profitability): Remove hardcoded min revenue map and add logic to consider gas cost #212

feat(profitability): Remove hardcoded min revenue map and add logic to consider gas cost #212

Conversation

kevinuma commented Jul 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinuma Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinuma Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai left a comment • edited Loading

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai commented Aug 3, 2022

kevinuma Jul 18, 2022 •

edited

Loading

kevinuma Jul 19, 2022 •

edited

Loading

nicholaspai left a comment •

edited

Loading