Skip to content
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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions rskj-core/src/main/java/co/rsk/RskContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -1860,7 +1860,7 @@ private MinGasPriceProvider getMinGasPriceProvider() {
if (minGasPriceProvider == null) {
long minGasPrice = getRskSystemProperties().minerMinGasPrice();
StableMinGasPriceSystemConfig stableGasPriceSystemConfig = getRskSystemProperties().getStableGasPriceSystemConfig();
minGasPriceProvider = MinGasPriceProviderFactory.create(minGasPrice, stableGasPriceSystemConfig);
minGasPriceProvider = MinGasPriceProviderFactory.create(minGasPrice, stableGasPriceSystemConfig, this::getEthModule);
}
logger.debug("MinGasPriceProvider type: {}", minGasPriceProvider.getType().name());
return minGasPriceProvider;
Expand Down Expand Up @@ -2222,4 +2222,4 @@ private void checkIfNotClosed() {
throw new IllegalStateException("RSK Context is closed and cannot be in use anymore");
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,27 @@ public class OnChainMinGasPriceSystemConfig {
public static final String ONCHAIN_STABLE_GAS_PRICE_CONFIG_PATH = "onChain";
//TODO property example
private static final String ADDRESS_PROPERTY = "address";
private static final String FROM_PROPERTY = "from";
private static final String DATA_PROPERTY = "data";
private final String address;
private final String from;
private final String data;

public OnChainMinGasPriceSystemConfig(Config config) {
this.address = config.getString(ADDRESS_PROPERTY);
address = config.getString(ADDRESS_PROPERTY);
from = config.getString(FROM_PROPERTY);
data = config.getString(DATA_PROPERTY);
}

public String getAddress() {
public String address() {
return address;
}

public String from() {
return from;
}

public String data() {
return data;
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package co.rsk.mine.gas.provider;

import co.rsk.config.mining.StableMinGasPriceSystemConfig;
import co.rsk.mine.gas.provider.onchain.OnChainMinGasPriceProvider;
import co.rsk.mine.gas.provider.onchain.OnChainMinGasPriceProviderFactory;
import co.rsk.mine.gas.provider.web.WebStableMinGasPriceProviderFactory;
import org.slf4j.Logger;
Expand All @@ -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) {
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

FixedMinGasPriceProvider fixedMinGasPriceProvider = new FixedMinGasPriceProvider(fixedMinGasPrice);
if (stableMinGasPriceSystemConfig == null) {
logger.warn("Could not find stable min gas price system config, using {} provider", fixedMinGasPriceProvider.getType().name());
Expand All @@ -34,7 +35,7 @@ public static MinGasPriceProvider create(long fixedMinGasPrice, StableMinGasPric
case WEB:
return WebStableMinGasPriceProviderFactory.create(stableMinGasPriceSystemConfig, fixedMinGasPriceProvider);
case ON_CHAIN:
return OnChainMinGasPriceProviderFactory.create(stableMinGasPriceSystemConfig, fixedMinGasPriceProvider);
return OnChainMinGasPriceProviderFactory.create(stableMinGasPriceSystemConfig, fixedMinGasPriceProvider, getContextCallback);
default:
logger.debug("Could not find a valid implementation for the method {}. Returning fallback provider {}", method, fixedMinGasPriceProvider.getType().name());
return fixedMinGasPriceProvider;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

public abstract class StableMinGasPriceProvider implements MinGasPriceProvider {
private static final Logger logger = LoggerFactory.getLogger("StableMinGasPrice");
private final MinGasPriceProvider fallBackProvider;
protected final MinGasPriceProvider fallBackProvider;

protected StableMinGasPriceProvider(MinGasPriceProvider fallBackProvider) {
this.fallBackProvider = fallBackProvider;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,35 @@
import co.rsk.mine.gas.provider.MinGasPriceProvider;
import co.rsk.mine.gas.provider.MinGasPriceProviderType;
import co.rsk.mine.gas.provider.StableMinGasPriceProvider;
import co.rsk.rpc.modules.eth.EthModule;
import co.rsk.util.HexUtils;
import org.ethereum.rpc.parameters.BlockIdentifierParam;
import org.ethereum.rpc.parameters.CallArgumentsParam;
import org.ethereum.rpc.parameters.HexAddressParam;
import org.ethereum.rpc.parameters.HexDataParam;
import org.ethereum.rpc.parameters.HexNumberParam;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class OnChainMinGasPriceProvider extends StableMinGasPriceProvider {
private final String address;
private static final Logger logger = LoggerFactory.getLogger(OnChainMinGasPriceProvider.class);

protected OnChainMinGasPriceProvider(MinGasPriceProvider fallBackProvider, OnChainMinGasPriceSystemConfig config) {
private final String toAddress;
private final String fromAddress;
private final String data;

@FunctionalInterface
public interface GetContextCallback {
EthModule getEthModule();
}
private final GetContextCallback getContextCallback;

protected OnChainMinGasPriceProvider(MinGasPriceProvider fallBackProvider, OnChainMinGasPriceSystemConfig config, GetContextCallback getContextCallback) {
super(fallBackProvider);
this.address = config.getAddress();
this.getContextCallback = getContextCallback;
this.toAddress = config.address();
this.fromAddress = config.from();
this.data = config.data();
}

@Override
Expand All @@ -20,10 +42,47 @@ public MinGasPriceProviderType getType() {

@Override
public Long getStableMinGasPrice() {
return null;
EthModule ethModule = this.getContextCallback.getEthModule();
if (ethModule == null) {
logger.error("Could not get eth module");
return fallBackProvider.getMinGasPrice();
}

CallArgumentsParam callArguments = new CallArgumentsParam(
new HexAddressParam(fromAddress),
new HexAddressParam(toAddress),
null,
null,
null,
null,
new HexNumberParam(ethModule.chainId()),
null,
new HexDataParam(data),
null
);
try {
String callOutput = ethModule.call(callArguments, new BlockIdentifierParam("latest"));

// Parse the output of the call to get the exchange rate. Will not work with bytes32 values!
return HexUtils.jsonHexToLong(
callOutput
);
} catch (Exception e) {
logger.error("Error calling eth module", e);

return fallBackProvider.getMinGasPrice();
}
}

public String getToAddress() {
return toAddress;
}

public String getFromAddress() {
return fromAddress;
}

public String getAddress() {
return address;
public String getData() {
return data;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Up @@ -8,7 +8,7 @@ public class OnChainMinGasPriceProviderFactory {
private OnChainMinGasPriceProviderFactory() {
}

public static OnChainMinGasPriceProvider create(StableMinGasPriceSystemConfig config, MinGasPriceProvider fallbackProvider) {
return new OnChainMinGasPriceProvider(fallbackProvider, config.getOnChainConfig());
public static OnChainMinGasPriceProvider create(StableMinGasPriceSystemConfig config, MinGasPriceProvider fallbackProvider, OnChainMinGasPriceProvider.GetContextCallback getContextCallback) {
return new OnChainMinGasPriceProvider(fallbackProvider, config.getOnChainConfig(), getContextCallback);
}
}
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
Expand Up @@ -22,7 +22,7 @@ void createFixedMinGasPriceProvider() {
);
StableMinGasPriceSystemConfig config = new StableMinGasPriceSystemConfig(testConfig);

MinGasPriceProvider provider = MinGasPriceProviderFactory.create(100L, config);
MinGasPriceProvider provider = MinGasPriceProviderFactory.create(100L, config, () -> null);

assertTrue(provider instanceof FixedMinGasPriceProvider);
assertEquals(100, provider.getMinGasPrice());
Expand All @@ -31,7 +31,7 @@ void createFixedMinGasPriceProvider() {

@Test
void createWithNullConfig() {
MinGasPriceProvider provider = MinGasPriceProviderFactory.create(100L, null);
MinGasPriceProvider provider = MinGasPriceProviderFactory.create(100L, null, () -> null);
assertTrue(provider instanceof FixedMinGasPriceProvider);
assertEquals(100, provider.getMinGasPrice());
assertEquals(MinGasPriceProviderType.FIXED, provider.getType());
Expand All @@ -52,7 +52,7 @@ void createWebProviderMethod() {

StableMinGasPriceSystemConfig disabledConfig = new StableMinGasPriceSystemConfig(testConfig);

MinGasPriceProvider provider = MinGasPriceProviderFactory.create(100L, disabledConfig);
MinGasPriceProvider provider = MinGasPriceProviderFactory.create(100L, disabledConfig, () -> null);

assertTrue(provider instanceof WebMinGasPriceProvider);
assertEquals(100, provider.getMinGasPrice());
Expand All @@ -74,7 +74,7 @@ void createWithDisabledConfigReturnFixed() {

StableMinGasPriceSystemConfig disabledConfig = new StableMinGasPriceSystemConfig(testConfig);

MinGasPriceProvider provider = MinGasPriceProviderFactory.create(100L, disabledConfig);
MinGasPriceProvider provider = MinGasPriceProviderFactory.create(100L, disabledConfig, () -> null);

assertTrue(provider instanceof FixedMinGasPriceProvider);
assertEquals(100, provider.getMinGasPrice());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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());
}

}