Skip to content

Conversation

@apexearth
Copy link
Collaborator

No description provided.

Copy link
Contributor

@shahthepro shahthepro left a comment

Choose a reason for hiding this comment

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

One obvious bug with usage of latestRound, latestAnswer should be used instead. Other than that, looks good to me

- `Currency` type
- ensure exchange rates exist in the appropriate places
- add `base` and `quote` to `ExchangeRate`
@apexearth
Copy link
Collaborator Author

While running this I see it saying around ~36 minutes to complete on my system. It's around 10 minutes slower than it used to be.

Maybe some sort of multicall or batching might help. Or ensuring we have a minimum span of time/blocks between rate refreshes. ( though we lose accuracy :( )

- uncomment the other processors
- comment on the quantity of exchange rates being processed
@apexearth
Copy link
Collaborator Author

After doing more testing I'm not sure this adds much time at all. It seems quite negligible. I took a dump of the results and reran it using the result set as a cache/datasource from JSON and the difference was very small. (frax-staking processor 58907 vs 58842)

@apexearth apexearth marked this pull request as ready for review October 11, 2023 00:13
@apexearth apexearth requested a review from nick October 11, 2023 00:13
Copy link
Contributor

@shahthepro shahthepro left a comment

Choose a reason for hiding this comment

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

Looks good to me. Few comments inline

pairs: [Currency, Currency][],
) => {
await Promise.all(
pairs.map(([base, quote]) => ensureExchangeRate(ctx, block, base, quote)),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can probably do some filtering here. If the event was for tokens frxETH and stETH, could probably just query those tokens' price on-chain. For the rest, can find the previous value in DB and use that. Will reduce the number of RPC calls, while ensuring we have a price for all assets at a given block.

But probably better to leave this as it is for now rather than complicating it. If we run into rate-limit issues or performance bottlenecks, we could consider doing it then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Problem is if we did that then we would be showing some of the values at wrong prices for the block numbers shown. For any block number where we have a value for a currency in our database we are going to need the exchange rate for it at the same block.

if (!vault) {
result.promises.push(
ensureExchangeRates(ctx, block, [
['ETH', 'USD'],
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: These could be constants as well. Same for other processors and their related files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what you mean.

@apexearth apexearth merged commit 34ffcae into main Oct 11, 2023
@apexearth apexearth deleted the exchange-rates branch October 11, 2023 15:14
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.

3 participants