-
Notifications
You must be signed in to change notification settings - Fork 266
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
feat(smgp): implements eth_call exchange rate provider #2454
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Remove this class and call the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is out of scope of this PR. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package co.rsk.config.mining; | ||
|
||
import com.typesafe.config.Config; | ||
import com.typesafe.config.ConfigFactory; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
class OnChainMinGasPriceSystemConfigTest { | ||
private OnChainMinGasPriceSystemConfig config; | ||
private String address = "0x77045E71a7A2c50903d88e564cD72fab11e82051"; | ||
private String from = "0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826"; | ||
private String data = "0x98d5fdca"; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
Config testConfig = ConfigFactory.parseString( | ||
"address=\"" + address + "\"\n" + | ||
"from=\"" + from + "\"\n" + | ||
"data=\"" + data + "\"" | ||
); | ||
config = new OnChainMinGasPriceSystemConfig(testConfig); | ||
} | ||
|
||
@Test | ||
void testAddress() { | ||
assertEquals(address, config.address()); | ||
} | ||
|
||
@Test | ||
void testFrom() { | ||
assertEquals(from, config.from()); | ||
} | ||
|
||
@Test | ||
void testData() { | ||
assertEquals(data, config.data()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,4 +64,4 @@ public Long getStableMinGasPrice() { | |
return 1L; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
package co.rsk.mine.gas.provider.onchain; | ||
|
||
import co.rsk.config.mining.OnChainMinGasPriceSystemConfig; | ||
import co.rsk.mine.gas.provider.MinGasPriceProvider; | ||
import co.rsk.mine.gas.provider.MinGasPriceProviderType; | ||
import co.rsk.util.HexUtils; | ||
import org.junit.jupiter.api.AfterEach; | ||
import org.junit.jupiter.api.Assertions; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import co.rsk.rpc.modules.eth.EthModule; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.NullSource; | ||
import org.junit.jupiter.params.provider.ValueSource; | ||
|
||
import static org.mockito.ArgumentMatchers.any; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
|
||
class OnChainMinGasPriceProviderTest { | ||
private final String oracle_address = "0xbffBD993FF1d229B0FfE55668F2009d20d4F7C5f"; | ||
private final String from_address = "0xbffBD993FF1d229B0FfE55668F2009d20d4F7C5f"; | ||
private final String data = "0x"; | ||
private final long fallback_minGasPrice_fake = 1234567890L; | ||
|
||
private EthModule ethModule_mock; | ||
private MinGasPriceProvider fallback_mock; | ||
private OnChainMinGasPriceSystemConfig onChainMinGasPriceSystemConfig_mock; | ||
|
||
private OnChainMinGasPriceProvider onChainMinGasPriceProvider; | ||
|
||
@BeforeEach | ||
public void beforeEach() { | ||
ethModule_mock = mock(EthModule.class); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: You could mock this class directly on the constructor and avoid this repetitive initialization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(ethModule_mock.chainId()).thenReturn("0x21"); | ||
|
||
fallback_mock = mock(MinGasPriceProvider.class); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: You could mock this class directly on the constructor and avoid this repetitive initialization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: You could mock this class directly on the constructor and avoid this repetitive initialization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(onChainMinGasPriceSystemConfig_mock.address()).thenReturn(oracle_address); | ||
when(onChainMinGasPriceSystemConfig_mock.from()).thenReturn(from_address); | ||
when(onChainMinGasPriceSystemConfig_mock.data()).thenReturn(data); | ||
|
||
|
||
onChainMinGasPriceProvider = new OnChainMinGasPriceProvider( | ||
fallback_mock, | ||
onChainMinGasPriceSystemConfig_mock, | ||
() -> ethModule_mock | ||
); | ||
} | ||
|
||
@AfterEach | ||
public void afterEach() { | ||
ethModule_mock = null; | ||
fallback_mock = null; | ||
} | ||
|
||
|
||
|
||
@ParameterizedTest | ||
@NullSource | ||
@ValueSource(strings = {"0x123", "0xabc"}) | ||
void constructorSetsFieldsCorrectly(String data_input) { | ||
MinGasPriceProvider fallbackProvider = mock(MinGasPriceProvider.class); | ||
OnChainMinGasPriceSystemConfig config = mock(OnChainMinGasPriceSystemConfig.class); | ||
|
||
when(config.address()).thenReturn("0xaddress"); | ||
when(config.from()).thenReturn("0xfrom"); | ||
when(config.data()).thenReturn(data_input); | ||
|
||
OnChainMinGasPriceProvider provider = new OnChainMinGasPriceProvider(fallbackProvider, config, () -> ethModule_mock); | ||
|
||
Assertions.assertEquals("0xaddress", provider.getToAddress()); | ||
} | ||
|
||
@Test | ||
void constructorSetsFieldsToNullWhenConfigReturnsNull() { | ||
MinGasPriceProvider fallbackProvider = mock(MinGasPriceProvider.class); | ||
OnChainMinGasPriceSystemConfig config = mock(OnChainMinGasPriceSystemConfig.class); | ||
|
||
when(config.address()).thenReturn(null); | ||
when(config.from()).thenReturn(null); | ||
when(config.data()).thenReturn(null); | ||
|
||
OnChainMinGasPriceProvider provider = new OnChainMinGasPriceProvider(fallbackProvider, config, () -> ethModule_mock); | ||
|
||
Assertions.assertNull(provider.getToAddress()); | ||
Assertions.assertNull(provider.getFromAddress()); | ||
Assertions.assertNull(provider.getData()); | ||
} | ||
|
||
@Test | ||
void getStableMinGasPrice_callsEthModulesCallMethod() { | ||
String expectedPrice = "0x21"; | ||
when(ethModule_mock.call(any(), any())).thenReturn(expectedPrice); | ||
|
||
Assertions.assertEquals( | ||
HexUtils.jsonHexToLong(expectedPrice), | ||
onChainMinGasPriceProvider.getStableMinGasPrice() | ||
); | ||
} | ||
|
||
@ParameterizedTest | ||
@NullSource | ||
@ValueSource(strings = {"", "0x"}) | ||
void getStableMinGasPrice_callsFallback_whenNoData(String data_input) { | ||
when(onChainMinGasPriceSystemConfig_mock.data()).thenReturn(data_input); | ||
|
||
Assertions.assertEquals( | ||
fallback_minGasPrice_fake, | ||
onChainMinGasPriceProvider.getStableMinGasPrice(), | ||
"For " + data_input + ": " | ||
); | ||
} | ||
|
||
|
||
@Test | ||
void getStableMinGasPrice_callsFallback_whenEthModuleIsNull() { | ||
Assertions.assertEquals( | ||
fallback_minGasPrice_fake, | ||
onChainMinGasPriceProvider.getStableMinGasPrice() | ||
); | ||
} | ||
|
||
@Test | ||
void getType_returnsOnChain() { | ||
Assertions.assertEquals(MinGasPriceProviderType.ON_CHAIN, onChainMinGasPriceProvider.getType()); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not pass the EthModule directly instead of passing this GetContextCallback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.