feat(smgp): implements eth_call exchange rate provider #2454
Conversation
jurajpiar
force-pushed
the
juraj/eth_call_stableGasPrice_multi_factory
branch
2 times, most recently
from
8b99f4a to
fd94286
Compare
jurajpiar
force-pushed
the
juraj/eth_call_stableGasPrice_multi_factory
branch
from
fd94286 to
782ccf8
Compare
jurajpiar
force-pushed
the
juraj/eth_call_stableGasPrice_multi_factory
branch
3 times, most recently
from
be6c603 to
18397fe
Compare
jurajpiar
force-pushed
the
juraj/eth_call_stableGasPrice_multi_factory
branch
from
18397fe to
3dab053
Compare
|
| @@ -14,7 +15,7 @@ public class MinGasPriceProviderFactory { | |||
| private MinGasPriceProviderFactory() { | |||
| } | |||
|
|
|||
| public static MinGasPriceProvider create(long fixedMinGasPrice, StableMinGasPriceSystemConfig stableMinGasPriceSystemConfig) { | |||
| public static MinGasPriceProvider create(long fixedMinGasPrice, StableMinGasPriceSystemConfig stableMinGasPriceSystemConfig, OnChainMinGasPriceProvider.GetContextCallback getContextCallback) { | |||
There was a problem hiding this comment.
Why not pass the EthModule directly instead of passing this GetContextCallback?
There was a problem hiding this comment.
When you pass a callback, that callback is only ever called once the ethmodule exists, as there are no logical paths to it otherwise. When you pass the output of the getEthModule function you then have a runtime circular dependency ie halting problem, because at the time of instantiation of the miner-related objects ethModule does not exist. And to instantiate ethModule you need to pass those miner-related objects.
There was a problem hiding this comment.
But I agree that this is not very "Java-esque". My JS background shows, and can't think of other non-disruptive way to achieve this.
There was a problem hiding this comment.
suggestion:
Remove this class and call the new OnChainMinGasPriceProvider(fallbackProvider, config.getOnChainConfig(), getContextCallback) on the MinGasPriceProviderFactory::create method, line 38. This is not adding much.
There was a problem hiding this comment.
This is out of scope of this PR.
|
|
||
| @BeforeEach | ||
| public void beforeEach() { | ||
| ethModule_mock = mock(EthModule.class); |
There was a problem hiding this comment.
suggestion:
You could mock this class directly on the constructor and avoid this repetitive initialization.
There was a problem hiding this comment.
I know. But the whole point of this is to ensure that your stubs and mocks and what not are always clean for each test case.
| ethModule_mock = mock(EthModule.class); | ||
| when(ethModule_mock.chainId()).thenReturn("0x21"); | ||
|
|
||
| fallback_mock = mock(MinGasPriceProvider.class); |
There was a problem hiding this comment.
suggestion:
You could mock this class directly on the constructor and avoid this repetitive initialization.
There was a problem hiding this comment.
I know. But the whole point of this is to ensure that your stubs and mocks and what not are always clean for each test case.
| when(fallback_mock.getType()).thenReturn(MinGasPriceProviderType.FIXED); | ||
| when(fallback_mock.getMinGasPrice()).thenReturn(fallback_minGasPrice_fake); | ||
|
|
||
| onChainMinGasPriceSystemConfig_mock = mock(OnChainMinGasPriceSystemConfig.class); |
There was a problem hiding this comment.
suggestion:
You could mock this class directly on the constructor and avoid this repetitive initialization.
There was a problem hiding this comment.
I know. But the whole point of this is to ensure that your stubs and mocks and what not are always clean for each test case.
Vovchyk
merged commit f7a212e
into
feature/include_stable_minGasPrice_provider
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: