Fetch gas price using low-level RPC method #321
Conversation
|
Alternatively, it can be implemented by extending the JsonRpcProvider class like following: // eslint-disable-next-line functional/no-classes
export class LegacyGasPriceRpcProvider extends ethers.JsonRpcProvider {
public async getGasPrice(): Promise<bigint> {
const value = await this.send('eth_gasPrice', []);
return getBigInt(value);
}
}I didn't prefer this approach because it would necessitate modifying nearly every file that references |
| @@ -443,6 +444,7 @@ describe(updateFeedsLoopsModule.processBatch.name, () => { | |||
| jest.spyOn(logger, 'info'); | |||
|
|
|||
| // Skip actions other than generating signed api urls. | |||
| jest.spyOn(gasPriceModule, 'fetchAndStoreGasPrice').mockImplementation(); | |||
There was a problem hiding this comment.
Mhh, why do these need to be mocked now?
There was a problem hiding this comment.
Yes it looks redundant but I wanted to add them because processBatch includes fetchAndStoreGasPrice. Should I remove them?
There was a problem hiding this comment.
Nah, leave them. I was just surprised why the test was passing, but I guess either Ethers made a noop call or silently did nothing 🤷
There was a problem hiding this comment.
Test was passing probably because fetchAndStoreGasPrice handles error by returning null when it fails. However, this spy on fetchAndStoreGasPrice is good to have because I encountered this error when I am playing with tests even though I couldn't produce it now.
src/gas-price/gas-price.test.ts
Outdated
| it('fetches and stores the gas price from RPC provider', async () => { | ||
| jest.spyOn(Date, 'now').mockReturnValue(dateNowMock); | ||
| jest.spyOn(provider, 'getFeeData').mockResolvedValueOnce({ gasPrice: ethers.parseUnits('10', 'gwei') } as any); | ||
| jest.spyOn(gasPriceModule, 'getGasPrice').mockResolvedValueOnce(ethers.parseUnits('10', 'gwei')); |
There was a problem hiding this comment.
I believe you can spy on the provider instead and this way you avoid the need to import the entirety of gas price module. (Similarly in the test below).
There was a problem hiding this comment.
Yep, that is what I did first. However, with the current approach, it is abstracted from provider and depends on getGasPrice which we already tested. I'm not good at tests 😄 I'll do what you suggest at the end of the conversation.
There was a problem hiding this comment.
I see the point, but I think the ugliness it brings is not worth it. We can have a similarly readable test with:
jest.spyOn(provider, 'send').mockResolvedValueOnce(ethers.toBeHex(ethers.parseUnits('10', 'gwei')));
| @@ -373,12 +365,22 @@ describe(getRecommendedGasPrice.name, () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe(fetchAndStoreGasPrice.name, () => { | |||
| describe(gasPriceModule.getGasPrice.name, () => { | |||
| it('returns gas price as bigint when internal rpc call returns a valid hex string', async () => { | |||
There was a problem hiding this comment.
Can you add one more test when the provider fails? I checked Ethereum RPC spec (and some other sources) and I think we can rely that when the API returns something it will be a hex string...
* Use block number from the RPC call * Add test for failed block number fetch
Closes #305