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

kevinuma · 2022-08-17T19:01:29Z

Originally reviewed in #212

nicholaspai

Left some comments. Overall great work I'm really excited to add this feature! I'm just being a bit cautious on it as it's high risk

src/clients/ProfitClient.ts

test/ProfitClient.ConsiderProfitability.ts

…to gas

pxrl · 2022-08-19T15:04:44Z

src/clients/ProfitClient.ts

-      cgPrices = await this.coingeckoPrices(Object.keys(l1Tokens));
+      const [maticTokenPrice, otherTokenPrices] = await Promise.all([
+        // Add WMATIC for gas cost calculations.
+        this.coingeckoPrices([WMATIC], "polygon-pos"),


Should this be conditional upon RELAYER_DESTINATION_CHAINS including Polygon?

Nope this is only to fetch WMATIC on Polygon for gas price. Plus it doesn't exist on other chains anyway

But the price of WMATIC is only needed if the bot is actually filling relays on Polygon, right? The reason I ask is that the RL bot is obviously doing this, but I'm not sure that's a valid assumption for non-RL relayers.

Hmm that is true. It does make the code a bit more complex to have WMATIC being optional when the relayer's enabledChainIds doesn't include Polygon though. For now I think an extra request to get gas price for Polygon might not be a deal breaker. We can address this in a followup PR if needed.

Yeah I can see it's a bit clunky to do in the middle. Happy to consider it as an optimisation later on, once things settle down.

package.json

src/clients/ProfitClient.ts

nicholaspai · 2022-08-19T15:32:36Z

src/clients/ProfitClient.ts

+
+  private constructRelayerFeeQuery(chainId: number, provider: Provider): relayFeeCalculator.QueryInterface {
+    // Fallback to Coingecko's free API for now.
+    // TODO: Add support for Coingecko Pro.


eventually we can just use the sdk-v2's CoingeckoClient and get the fallback logic for free

SG. We can bump the sdk-v2 version when that's ready

It's ready right now but I want to isolate only the Vercel API to using CG pro to reduce pro requests for time being, basic seems to work for the most part for relayer

src/common/ClientHelper.ts

src/relayer/RelayerClientHelper.ts

nicholaspai

OOC, why are you changing the test syntax and modifying arrow function syntax and async/sync? Is something not working locally?

src/clients/ProfitClient.ts

nicholaspai · 2022-08-19T15:43:21Z

src/clients/ProfitClient.ts

+
+  private constructRelayerFeeQuery(chainId: number, provider: Provider): relayFeeCalculator.QueryInterface {
+    // Fallback to Coingecko's free API for now.
+    // TODO: Add support for Coingecko Pro.


It's ready right now but I want to isolate only the Vercel API to using CG pro to reduce pro requests for time being, basic seems to work for the most part for relayer

src/relayer/RelayerClientHelper.ts

test/ProfitClient.ConsiderProfitability.ts

nicholaspai

LGTM, i'm gonna request from @mrice32 too since this is a sensitive PR!

pxrl

Driveby comments

pxrl · 2022-08-19T16:16:19Z

src/clients/ProfitClient.ts

-      cgPrices = await this.coingeckoPrices(Object.keys(l1Tokens));
+      const [maticTokenPrice, otherTokenPrices] = await Promise.all([
+        // Add WMATIC for gas cost calculations.
+        this.coingeckoPrices([WMATIC], "polygon-pos"),


But the price of WMATIC is only needed if the bot is actually filling relays on Polygon, right? The reason I ask is that the RL bot is obviously doing this, but I'm not sure that's a valid assumption for non-RL relayers.

pxrl · 2022-08-19T19:40:35Z

src/clients/ProfitClient.ts

+    switch (chainId) {
+      case 1:
+        return new relayFeeCalculator.EthereumQueries(
+          provider,
+          undefined,
+          undefined,
+          undefined,
+          undefined,
+          coingeckoProApiKey,
+          this.logger,
+          gasMarkup
+        );


This is just a style comment, but is there any possibility for a construction something like the following?

const queryHandlers: { [chainId: number]: queryFunction } = { 1: relayFeeCalculator.EthereumQueries, 10: relayFeeCalculator.OptimismQueries, 137: relayFeeCalculator.PolygonQueries, 288: relayFeeCalculator.BobaQueries, 42161: relayFeeCalculator.ArbitrumQueries, } const queryFn: queryFunction = queryHandlers[chainId]; if (queryFn === undefined) { throw new Error(`Unexpected chain ${chainId}`); } return new queryFn( provider, undefined, undefined, undefined, undefined, coingeckoProApiKey, this.logger, gasMarkup );

Advantage would be that it's a 1-line change to add a new chain, and the overhead of changing the structure of the function arguments is quite a bit lower. It's also arguably easier to audit and ensure that all chains are supplying the same arguments in the same order.

I've used it a lot in C and Python, but I'm not sure how common it is in TS/JS though. Does it make sense here?

Amazing suggestion! It's much cleaner and more elegant. I updated the code.

mrice32

All of this logic looks great! Really excited about rolling this out. A few very minor comments and nits.

mrice32 · 2022-08-19T21:31:42Z

src/clients/ProfitClient.ts

+  1: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH
+  10: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH


nit: why not make WETH a constant, like WMATIC?

mrice32 · 2022-08-19T21:32:51Z

src/clients/ProfitClient.ts

+    const totalGasCostWei = this.getTotalGasCost(deposit.destinationChainId);
+    if (!totalGasCostWei) {
+      const chainId = deposit.destinationChainId;
+      const errorMsg = `Missing total gas cost for ${chainId}. This likely indicate some gas cost request failed`;


Suggested change

const errorMsg = `Missing total gas cost for ${chainId}. This likely indicate some gas cost request failed`;

const errorMsg = `Missing total gas cost for ${chainId}. This likely indicates some gas cost request failed`;

mrice32 · 2022-08-19T21:34:48Z

src/clients/ProfitClient.ts

+    const totalGasCostWei = this.getTotalGasCost(deposit.destinationChainId);
+    if (!totalGasCostWei) {


Could this ever be falsy? It looks like the method defaults to returning a BigNumber of 0 if there is no price stored, and I don't think a BigNumber object can be falsy (please correct me if I'm wrong!).

Good call! Updated to be more explicit and check against undefined + toBN(0)

mrice32 · 2022-08-19T21:37:44Z

src/clients/ProfitClient.ts

+      this.logger.error({
+        at: "ProfitClient",
+        message: errorMsg,
+        allGasCostsFetched: this.totalGasCosts,
+        chainId,
+      });
+      throw new Error(errorMsg);


Should we log an error and throw an error? Not necessarily wrong, but I think this will end up generating two error alerts. One from the crash of the process and one from the error that you explicitly send here. Also, a crash will generally be retried, so if the issue is sporadic, the retries will mean that no human gets alerted, but this error log will immediately alert someone.

I changed the logging to warn and still leave throw new Error so the relayer would fail.

mrice32 · 2022-08-19T21:42:06Z

src/clients/ProfitClient.ts

    if (toBN(deposit.relayerFeePct).eq(toBN(0))) {
      this.logger.debug({ at: "ProfitClient", message: "Deposit set 0 relayerFeePct. Rejecting relay" });
      return false;
    }

+    // This should happen before the previous checks as we don't want to turn them off when profitability is disabled.


nit: I'm a little confused by this comment. Should this check happen before or after the checks above? If it should happen before them, then shouldn't we move it?

So sorry I had a brain fart. It should be "after"

mrice32 · 2022-08-19T21:46:50Z

src/clients/ProfitClient.ts

-      cgPrices = await this.coingeckoPrices(Object.keys(l1Tokens));
+      const [maticTokenPrice, otherTokenPrices] = await Promise.all([
+        // Add WMATIC for gas cost calculations.
+        this.coingeckoPrices([WMATIC], "polygon-pos"),


Can we use the ETH mainnet matic token here instead, so we don't need to do this special casing for matic? We could then do all of our queries with ethereum as the platform, I think, right?

I think that might also simplify some of the logic below.

Good find! I updated to fetch MATIC price on ethereum

mrice32 · 2022-08-19T21:49:44Z

src/clients/ProfitClient.ts

+        this.logger.error({ at: "ProfitClient", message: "Could not fetch all token prices 💳", mrkdwn });
+        throw new Error(mrkdwn);


Same comment as above -- are we sure we want to alert and crash?

Changed the logging to warn

mrice32 · 2022-08-19T21:53:38Z

src/clients/ProfitClient.ts

+    const getGasCosts = [];
+    for (const chainId of this.enabledChainIds) {
+      getGasCosts.push(this.relayerFeeQueries[chainId].getGasCosts());


nit: Is there some reason we've spread the looping and Promise.all out? Could we just do it in a simple Promise.all wrapping a map?

const gasCosts = await Promise.all(this.enabledChainIds.map( (chainId) => this.relayerFeeQueries[chainId].getGasCosts() ));

Good suggestion! Updated

kevinuma added 7 commits August 17, 2022 11:33

improve(relayer): Revamp profitability to consider gas costs

a2fe607

fix

7449882

fix

5ad514f

fix

f59e6d8

fix

7992287

fix

c966c21

test

db9e7b5

kevinuma changed the title ~~Gas~~ feat(relayer): Consider gas cost when calculating profitability Aug 18, 2022

nicholaspai reviewed Aug 18, 2022

View reviewed changes

src/clients/ProfitClient.ts Show resolved Hide resolved

src/clients/ProfitClient.ts Outdated Show resolved Hide resolved

test/ProfitClient.ConsiderProfitability.ts Outdated Show resolved Hide resolved

test/ProfitClient.ConsiderProfitability.ts Show resolved Hide resolved

nicholaspai requested review from pxrl, mrice32 and chrismaree August 18, 2022 12:16

kevinuma added 3 commits August 18, 2022 09:24

commit

77d0813

Merge branch 'master' of github.com-uma:across-protocol/relayer-v2 in…

77df100

…to gas

tests

95f578f

kevinuma marked this pull request as ready for review August 19, 2022 14:55

kevinuma requested a review from nicholaspai August 19, 2022 14:56

pxrl reviewed Aug 19, 2022

View reviewed changes

nicholaspai reviewed Aug 19, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

nicholaspai reviewed Aug 19, 2022

View reviewed changes

src/clients/ProfitClient.ts Outdated Show resolved Hide resolved

nicholaspai reviewed Aug 19, 2022

View reviewed changes

src/clients/ProfitClient.ts Show resolved Hide resolved

nicholaspai reviewed Aug 19, 2022

View reviewed changes

src/clients/ProfitClient.ts Outdated Show resolved Hide resolved

nicholaspai reviewed Aug 19, 2022

View reviewed changes

src/clients/ProfitClient.ts Outdated Show resolved Hide resolved

nicholaspai reviewed Aug 19, 2022

View reviewed changes

src/clients/ProfitClient.ts Outdated Show resolved Hide resolved

nicholaspai reviewed Aug 19, 2022

View reviewed changes

src/clients/ProfitClient.ts Show resolved Hide resolved

nicholaspai reviewed Aug 19, 2022

View reviewed changes

src/common/ClientHelper.ts Show resolved Hide resolved

Comments

372ca15

nicholaspai reviewed Aug 19, 2022

View reviewed changes

src/relayer/RelayerClientHelper.ts Show resolved Hide resolved

merge

6904731

nicholaspai reviewed Aug 19, 2022

View reviewed changes

kevinuma added 5 commits August 19, 2022 11:14

update yarn.lock

3e560d6

commit

77a6f47

fix tests

418064a

fix

1603a53

comments

60de221

kevinuma requested a review from nicholaspai August 19, 2022 17:11

nicholaspai reviewed Aug 19, 2022

View reviewed changes

test/ProfitClient.ConsiderProfitability.ts Outdated Show resolved Hide resolved

nicholaspai approved these changes Aug 19, 2022

View reviewed changes

Comments

ea87a2e

kevinuma requested a review from pxrl August 19, 2022 18:16

kevinuma and others added 3 commits August 19, 2022 13:45

Merge branch 'master' into gas

a06fef8

logging

b06b012

logs

862071d

pxrl reviewed Aug 19, 2022

View reviewed changes

comments

b1a5127

kevinuma requested a review from pxrl August 19, 2022 19:52

mrice32 reviewed Aug 19, 2022

View reviewed changes

comments

f23129e

kevinuma merged commit 14e6d2d into master Aug 20, 2022

kevinuma mentioned this pull request Aug 21, 2022

fix(profit client): Allow fetching CG prices when relay gas profitability is disabled #266

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kevinuma commented Aug 17, 2022 •

edited

Loading

nicholaspai left a comment •

edited

Loading

pxrl Aug 19, 2022

kevinuma Aug 19, 2022

pxrl Aug 19, 2022

kevinuma Aug 19, 2022

pxrl Aug 19, 2022

nicholaspai Aug 19, 2022

kevinuma Aug 19, 2022

nicholaspai Aug 19, 2022

nicholaspai left a comment

nicholaspai Aug 19, 2022

nicholaspai left a comment

pxrl left a comment

pxrl Aug 19, 2022

pxrl Aug 19, 2022 •

edited

Loading

kevinuma Aug 19, 2022

mrice32 left a comment

mrice32 Aug 19, 2022

kevinuma Aug 19, 2022

mrice32 Aug 19, 2022

kevinuma Aug 19, 2022

mrice32 Aug 19, 2022

kevinuma Aug 19, 2022

mrice32 Aug 19, 2022

kevinuma Aug 19, 2022 •

edited

Loading

mrice32 Aug 19, 2022

kevinuma Aug 19, 2022

mrice32 Aug 19, 2022

kevinuma Aug 19, 2022

mrice32 Aug 19, 2022

kevinuma Aug 19, 2022

mrice32 Aug 19, 2022

kevinuma Aug 19, 2022

		1: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH
		10: "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2", // WETH

	const errorMsg = `Missing total gas cost for ${chainId}. This likely indicate some gas cost request failed`;
	const errorMsg = `Missing total gas cost for ${chainId}. This likely indicates some gas cost request failed`;

		const totalGasCostWei = this.getTotalGasCost(deposit.destinationChainId);
		if (!totalGasCostWei) {

		this.logger.error({ at: "ProfitClient", message: "Could not fetch all token prices 💳", mrkdwn });
		throw new Error(mrkdwn);

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

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

Conversation

kevinuma commented Aug 17, 2022 • edited Loading

nicholaspai left a comment • 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

nicholaspai left a 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

pxrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxrl Aug 19, 2022 • edited Loading

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

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 Aug 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

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 commented Aug 17, 2022 •

edited

Loading

nicholaspai left a comment •

edited

Loading

pxrl Aug 19, 2022 •

edited

Loading

kevinuma Aug 19, 2022 •

edited

Loading