Configurable Fiat-Based Stable MinGasPrice

[asoto-iov]

Enhancing Miner Configuration for Stable Transaction Costs

Description

Develop and integrate a feature that allows RSK miners to configure the minimum gas price (MinGasPrice) in fiat currency instead of the current cryptocurrency (WEI) linked to Bitcoin. This feature aims to stabilize the transaction cost in fiat terms, improving predictability and user experience.

Motivation and Context

Currently, RSK miners can set the minimum gas price (MinGasPrice) using the miner.minGasPrice configuration, which is denominated in WEI and inherently linked to the Bitcoin price. This linkage causes transaction costs to fluctuate with Bitcoin's value, potentially leading to increased costs when Bitcoin appreciates, thereby adversely affecting user experience.
To address this issue, the proposed feature aims to empower miners with the ability to specify and configure the minimum gas price in fiat currency. This change would stabilize transaction costs in fiat terms, regardless of cryptocurrency market variations, and is intended to enhance predictability and user satisfaction. Currently, the system supports only one configuration parameter for setting the MinGasPrice.

The goal is to give miners a capability to specify / configure min gas price value denominated in a fiat currency.

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• Tests for the changes have been added (for bug fixes / features)
• Requires Activation Code (Hard Fork)

• Other information:

[fmacleal]

I really liked the approach and the way you tackled the problem. Good job.

Generally, I don't have big changes to recommend. But some small things that IMO could make it even more flexible than the approach you have followed. Let me know your opinion, if you want to discuss it on slack, ping me. 😄

[jurajpiar]

This seems unnecessarily complicated. Why should a strategy know it's type? Imo there is no need for a factory and all those different abstractions. Also there is no need to change the MinimumGasPriceCalculator. I'd simplify it to the following, given that all the information needed is known on boot-up:

classDiagram
direction RL

class MinimumGasPriceCalculator {
    Unchanged - only to illustrate use
    +construcor(long)
}
class BlockToMineBuilder {
    Unchanged - only to illustrate use
    +constuctor(..., MinimumGasPriceCalculator)
}
RskContext --* BlockToMineBuilder
BlockToMineBuilder --o MinimumGasPriceCalculator
RskContext --* MinimumGasPriceCalculator

class RskSystemProperties {
    +getMinerMinGasPriceConfig(): MinGasPriceConfig
}

class RskContext {
    -getMiningConfig(): MiningConfig
    +getBlockToMineBuilder(): BlockToMineBuilder
}

class MinGasPriceConfig {
    +providerType: "onchain" | "http" | "default"
    +url: URL
    +minGasPriceTarget: long
    +otherStuffIfAny
    +createMinGasPriceProvider(MinGasPriceConfig): MinGasPriceProvider
}

class MiningConfig {
    -minGasPriceProvider: MinGasPriceProvider
    +constructor(MinGasPriceConfig)
    +getMinGasPriceTarget(): long
}

class MinGasPriceProvider {
    <<<Interface>>>
    +getMinGasPrice()
}

class DefaultMinGasPriceProvider {
    +constructor()
}

class OffChainMinGasPriceProvider {
    +constructor(OffChainMinGasPriceProviderConfig)
}

class OnChainMinGasPriceProvider {
    +constructor(OnChainMinGasPriceProviderConfig)
}


MinGasPriceConfig *-- RskSystemProperties
RskSystemProperties *-- RskContext

MinGasPriceProvider *-- MinGasPriceConfig
MinGasPriceConfig <-- MiningConfig
MinGasPriceProvider o-- MiningConfig
MiningConfig *-- RskContext


MinGasPriceProvider <|-- DefaultMinGasPriceProvider
MinGasPriceProvider <|-- OffChainMinGasPriceProvider
MinGasPriceProvider <|-- OnChainMinGasPriceProvider

Loading

clarified through discussion

[fmacleal]

Good job. I just added some comments with suggestions and thoughts. Let me know if you have some doubt, you can reply here or reach me on slack.

[fmacleal]

question: why do you need two classes so similar to hold these informations? Why can't you use the one for mapping WebStableMinGasSystemConfig to receive these parameters on the WebMinGasPriceProvider?

[fmacleal]

suggestion: This whole class is unnecessary, it's basically calling the constructor from the OnChainMinGasPriceProvider. If, big if here, we have multiple ways to retrieve the price on-chain we could create such factory. It's the same idea from the other factory, we could just call the constructor directly, we have all the info needed on the other factory.

[fmacleal]

suggestion: This whole class is unnecessary, it's basically calling the constructor from the WebMinGasPriceProvider. If, big if here, we have multiple ways to retrieve the price off-chain we could create such factory. But at the moment, I would vouch to keep it simple and write so flexible code if we really need in the future. Having just a simple MinGasPriceProviderFactory is enough, and we can add the different ways of retrieve here.

[fmacleal]

thought: maybe would be good to have logged here as well that the feature isn't enabled.

[fmacleal]

suggestion: as commented previously, I would create the MinGasPriceProvider implementation here directly according the method. You already have all the configurations here, you could have methods to get the config according each type passed to extract the configuration complexity creation.

The concept of a factory is exactly hold these logics necessary to return different implementations, if start to become too complex, we can abstract more and create other classes. But for now, the first level of abstraction is more than enough.

[jurajpiar]

We should be careful with mixing problem domains. I suspect that the RBTC/USD exchange rate != min gas price!
And so all these classes should to be renamed. Furthermore, I don't see anywhere where we make this conversion from XR to min gas price.
What am I missing?

[jurajpiar]

I know I suggested this, but that was when I thought that the on chain call would be done directly in the vm rather than through eth module. And so, we should call it ETH_CALL.

[sonarcloud]

Quality Gate passed

Issues
15 New issues
0 Accepted issues

Measures
0 Security Hotspots
77.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

fix for time units usage is needed

[sonarcloud]

Quality Gate passed

Issues
15 New issues
0 Accepted issues

Measures
0 Security Hotspots
77.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud