Gas price #26
Gas price #26
Conversation
|
Please re-review based on the new spec 😄 The changes include:
|
src/gas-price/gas-price.ts
Outdated
| }); | ||
|
|
||
| // Get the provider recommended gas price | ||
| const goGasPrice = await go(async () => provider.getGasPrice(), { retries: 1, attemptTimeoutMs: 2000 }); |
There was a problem hiding this comment.
Not sure how I feel about retrying the get gas price call. It seems a bit arbitrary to me.
There was a problem hiding this comment.
Errors should probably be handled at the getAirseekerV2ProviderRecommendedGasPrice level in the main loop, with no go around the getGasPrice if we don't want to retry there.
getGasPrice could even be called outside of this scope of these functions (in the main loop) and the value passed to these functions, making them all synchronous if we wanted? I don't have a big preference either way since it's pretty simple.
|
|
||
| export const gasPriceStore: Record<string, Record<string, GasState>> = {}; | ||
|
|
||
| export const initializeGasStore = (chainId: string, providerName: string) => { |
There was a problem hiding this comment.
I think we should have just a single state, so I'd move merge this logic with src/state.
There was a problem hiding this comment.
Done, that was quite a refactor 😄
There was a problem hiding this comment.
Hehe, we can plug in https://immerjs.github.io/immer/ to help simplify this. Feel free to create an issue.
| if ( | ||
| lastDataFeedValue && | ||
| newDataFeedUpdateOnChainValues && | ||
| lastDataFeedValue?.value === newDataFeedUpdateOnChainValues.newDataFeedValue.value && |
There was a problem hiding this comment.
In this slack comment I keep wondering about this.
We will be making new transactions all the time (since the singed data changes asynchronously) and this will not track pending txs correctly. I do not see any other way except relying on wallet nonces or awaiting the transaction promise.
There was a problem hiding this comment.
I guess the important part is that if the on-chain value changes there had to be an update. If that's only what we care about then the logic is sound.
I was afraid that we don't know how old is the timestamp (of the value that caused the update), but now that I think about it we don't care about that.
There was a problem hiding this comment.
I really like these test btw.
There was a problem hiding this comment.
🙏 Thanks for the discussions and making this work.
With https://github.com/api3dao/airseeker-v2/pull/26/files#r1372062174 I think the logic is now clear to me.
Closes #10