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

Moved fee estimation and profitability calculation to /core #82

Merged
merged 6 commits into from
Feb 3, 2022

Conversation

zoey-kaiser
Copy link
Contributor

closes #80

After some trying around I decided to only memoizee the functions. Moving them out of the store would require a larger rewrite of how they are imported and merged into the Auction Transaction.

If this is not okay, then I can sit down and refractor them too!

@valiafetisov
Copy link
Contributor

valiafetisov commented Feb 2, 2022

We have to move them to the core, since we want to use them in the bot-twitter, that's the whole point of this refactoring. After they are moved to the core, you will no longer need store/fee file and can directly enrich the auction with fees like all other enrichers.

And, as always, it's better to cache lower-level function that does the fetching (getGasPrice or even the one above) instead.

@zoey-kaiser
Copy link
Contributor Author

We have to move them to the core, since we want to use them in the bot-twitter, that's the whole point of this refactoring. After they are moved to the core, you will no longer need store/fee file and can directly enrich the auction with fees like all other enrichers.

The functions to calculate the fee etc are already in the core. The store only temporary saves them for auction injecting, but all of the needed functions are already in the core.

https://github.com/sidestream-tech/unified-auctions-ui/blob/main/core/src/fees.ts
https://github.com/sidestream-tech/unified-auctions-ui/blob/main/core/src/price.ts#L54

Or am I mistaken here?

@valiafetisov
Copy link
Contributor

The functions to calculate the fee etc are already in the core

Perfect, then let's add cache and get rid of the store?

core/src/fees.ts Outdated
Comment on lines 29 to 33
export const getApproximateTransactionFees = memoizee(_getApproximateTransactionFees, {
maxAge: TRANSACTION_FEE_CACHE,
promise: true,
length: 1,
});
Copy link
Contributor

@valiafetisov valiafetisov Feb 3, 2022

Choose a reason for hiding this comment

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

  • As suggested above: can you please cache actual function that fetches gas price instead (and please use smaller cache, something like 10 seconds)?

  • getExchangeRateBySymbol is already cached, as far as I remember, so you wouldn't need to cache cached output, otherwise it can pile up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • As suggested above: can you please cache actual function that fetches gas price instead (and please use smaller cache, something like 10 seconds)?
  • Cached the Gas price for 10 seconds
  • Removed cache on parent function

core/src/gas.ts Outdated

const API_URL = 'https://ethgasstation.info/json/ethgasAPI.json';
const TRANSACTION_SPEED = 'fast';

const getCurrentGasPrice = async function (): Promise<BigNumber> {
const GAS_CACHE = 10 * 60 * 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

That is 10 minutes 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh god.. Hahaha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set it to 10 seconds now!

@valiafetisov valiafetisov merged commit bb2c0a4 into main Feb 3, 2022
@valiafetisov valiafetisov deleted the move-functions-to-core branch February 3, 2022 12:11
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.

Move fee estimation and profitability calculation to /core
2 participants