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

feat: add fee detector #338

Closed
wants to merge 9 commits into from
Closed

feat: add fee detector #338

wants to merge 9 commits into from

Conversation

marktoda
Copy link
Contributor

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • What is the current behavior? (You can also link to an open issue here)

  • What is the new behavior (if this is a feature change)?

  • Other information:

@marktoda marktoda force-pushed the add-fee-detector branch 2 times, most recently from b8ff275 to 913f64e Compare August 22, 2023 23:18
@marktoda marktoda marked this pull request as ready for review August 23, 2023 00:09
@marktoda
Copy link
Contributor Author

> validation.getFeesByToken(new Token(1, '0xaBeC00542D141BDdF58649bfe860C6449807237c', 18))
{
  buyFeeBps: BigNumber { _hex: '0x64', _isBigNumber: true },
  sellFeeBps: BigNumber { _hex: '0x64', _isBigNumber: true }
}
>

Copy link
Contributor

@zhongeric zhongeric left a comment

Choose a reason for hiding this comment

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

Lgtm, are we planning on deprecating the old token-validator-provider? Also, are we planning on adding this to AlphaRouter or just have it exported for use in i.e. frontend?

src/abis/TokenFeeDetector.json Outdated Show resolved Hide resolved
@jsy1218
Copy link
Member

jsy1218 commented Aug 23, 2023

Lgtm, are we planning on deprecating the old token-validator-provider? Also, are we planning on adding this to AlphaRouter or just have it exported for use in i.e. frontend?

From the slack thread, it sounds like interface will use it as FOT workaround to begin with. routing team still evaluating whether we need to take on quoting FOT after we mitigate the quote latencies.

src/providers/token-fee-provider.ts Outdated Show resolved Hide resolved
@marktoda
Copy link
Contributor Author

Lgtm, are we planning on deprecating the old token-validator-provider

The old one still serves a different purpose! it categorizes based on whether the token has a fee at all, or fails on transfer. It also allows checking against multiple bases, which doesn't make as much sense here because which fee would we show if it was different depending on the base

Copy link
Contributor

@andysmith415 andysmith415 left a comment

Choose a reason for hiding this comment

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

Appreciate you jumping on this @marktoda! Handful of comments, feel free to ping me for any questions; happy to elaborate.

src/providers/token-fee-provider.ts Outdated Show resolved Hide resolved
src/providers/token-fee-provider.ts Outdated Show resolved Hide resolved
src/providers/token-fee-provider.ts Outdated Show resolved Hide resolved
src/providers/token-fee-provider.ts Show resolved Hide resolved
src/providers/token-fee-provider.ts Outdated Show resolved Hide resolved
src/providers/token-fee-provider.ts Outdated Show resolved Hide resolved
src/providers/token-fee-provider.ts Show resolved Hide resolved
src/providers/token-fee-provider.ts Outdated Show resolved Hide resolved
src/providers/token-fee-provider.ts Outdated Show resolved Hide resolved
Comment on lines +159 to +163
return {
getFeesByToken: (token: Token) =>
tokenToResult[token.address.toLowerCase()],
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not a fan of returning anonymous functions as part of the response rather than objects of a specific class.

Could we instead do:

export class TokenFeeResults {
  constructor(private readonly tokenToResult: { [tokenAddress: string]: TokenFeeResult }) {}
  
  getFeesByToken(token: Token): TokenFeeResult | undefined {
    return this.tokenToResult[token.address.toLowerCase()];
  }
}

and here return:

  return new TokenFeeResults(tokenToResult);

constructor(
private chainId: ChainId,
private multicall2Provider: IMulticallProvider,
private tokenFeeCache: ICache<TokenFeeResult>,
Copy link
Member

Choose a reason for hiding this comment

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

As I'm working on the router backend support for FOT, I realized that this TokenFeeProvider is tightly coupled with the in-memory ICache. We will need both in-memory memory (for client-side fallback), as well as dynamo caching (for routing-api distributed caches).

I think I will create another PR, meanwhile leveraging the insights and feedbacks from this PR.

@marktoda marktoda closed this Sep 5, 2023
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.

5 participants