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

Gas prediction implementation #514

Merged
merged 21 commits into from
Apr 14, 2022
Merged

Conversation

yaroslavyaroslav
Copy link
Collaborator

@yaroslavyaroslav yaroslavyaroslav commented Mar 31, 2022

Gas prediction

This PR implements base functionality of gas prediction.

What is it

The prediction calculates given percentiles (e.g. percent of how value is high against all dataset values) of a last n blocks data.

How to use it

It brings Oracle object that is encapsulating all prediction methods. To use it you need to create Oracle object as shown bellow:

let web3 = Web3.InfuraMainnetWeb3(accessToken: Constants.infuraToken)

let block: BlockNumber = .exact(14571792)

let blockCount: BigUInt = 20

let percentiles: [Double] = [25, 50, 75]

var oracle = Web3.Oracle(web3, block: block, blockCount: blockCount, percentiles: percentiles)

After initializing, since it’s a class you’ll be able to use in all around of your app, but since it’s not singleton yet you’re on your own to manage its lifecycle.

API

Oracle object provides setup API are follow

After creation you’ll be able to get its prediction based on its settings. Also Oracle object have follow property to be setup within its lifecycle:

  • block: Block to start getting history backward
  • blockCount: Count of blocks to include in dataset
  • percentiles: Sets values by which dataset would be sliced.

If you set percentiles property to [25.0, 50.0, 75.0] on any prediction property read you'll get [71456911562, 92735433497, 105739785122] which means that first value in array is more than 25% of the whole dataset, second one more than 50% of the dataset and third one more than 75% of the dataset.

Oracle object provides prediction API are follow

  • baseFeePercentiles: [BigUInt] - Soften baseFee amount.
  • tipFeePercentiles: [BigUInt] - Tip amount.
  • bothFeesPercentiles: (baseFee: [BigUInt], tip: [BigUInt])? - Summary fees amount.
  • gasPriceLegacyPercentiles: [BigUInt] - Legacy gasPrice amount.

All arrays have the same structure: [percentile_1, percentile_2, percentile_3, ...].count == self.percentile.count or empty array if failed to predict. By default there's 3 percentile.

@yaroslavyaroslav yaroslavyaroslav marked this pull request as draft March 31, 2022 19:26
@yaroslavyaroslav yaroslavyaroslav marked this pull request as ready for review April 5, 2022 09:42
@yaroslavyaroslav yaroslavyaroslav marked this pull request as draft April 5, 2022 09:46
@yaroslavyaroslav yaroslavyaroslav changed the title Merge branch 'develop' into feature/gas-prediction Gas prediction implementation Apr 5, 2022
Copy link
Contributor

@mloit mloit left a comment

Choose a reason for hiding this comment

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

Almost all of these corrections are simple typos in the comments. I know English is not your first language, so these are not meant as a comment you or your abilities. Otherwise the code looks good so far, with only minor suggestion.

Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
Sources/web3swift/Web3/Web3+GasOracle.swift Outdated Show resolved Hide resolved
@mloit
Copy link
Contributor

mloit commented Apr 6, 2022

predictGasPriceLegacy(...) ... Should be used only on pre EIP-1559 blocks.

Not sure what you mean by this. Legacy transactions (and EIP-2930) only have gasPrice and gasLimit parameters, Both still exist in blocks today along with EIP-1559 transactions in the same block. Obviously EIP-1159 transactions (and blocks) can not exist before the London fork, but all types of transactions still exist after the fork.

If this function is only for getting gasPrice estimation on pre-fork blocks, how do we get gasPrice after the fork to support legacy and EIP-2930 type transactions?

case .maximum:
// Checking that suggestedBaseFee are not lower than will be in the next block
// because in tne maximum statistic we should guarantee that transaction would be included in it.
return max(calcBaseFee(for: latestBlock), noAnomalyArray.max()!)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should not use force unwrap

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please pay attention that at line 51 its explicitly checks that array is not empty, therefore it's acceptable to use forse unwrap, since each function beyond returns nil only if arrays is empty.


extension Array {
func cropAnomalyValues() -> Self {
var tmpArr = self.dropFirst()
Copy link
Collaborator

Choose a reason for hiding this comment

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

what makes the first and last values anomalous?

Copy link
Contributor

Choose a reason for hiding this comment

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

in statistics the extreme outliers in a set are often looked at to be deviations from the norm, or anomalies. So by clipping them off, you can often get a more meaningful result. In this case the list is sorted,so first and last entries would contain the extreme outliers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right about not enough encapsulation here. I've include sorting to this method

@yaroslavyaroslav
Copy link
Collaborator Author

predictGasPriceLegacy(...) ... Should be used only on pre EIP-1559 blocks.

Not sure what you mean by this. Legacy transactions (and EIP-2930) only have gasPrice and gasLimit parameters, Both still exist in blocks today along with EIP-1559 transactions in the same block. Obviously EIP-1159 transactions (and blocks) can not exist before the London fork, but all types of transactions still exist after the fork.

If this function is only for getting gasPrice estimation on pre-fork blocks, how do we get gasPrice after the fork to support legacy and EIP-2930 type transactions?

I still insist that on after London transactions should be used predictTip or predictBaseFee methods coz it'll give some accurate values. But predictGasPriceLegacy will also return valid values on post London blocks, but it'll be useless for transaction fee prediction since there's no way to get proportion to split that sum. So this comment is awareing method user, that this method is not designed to be used in post London blocks.

@mloit
Copy link
Contributor

mloit commented Apr 7, 2022

I still insist that on after London transactions should be used predictTip or predictBaseFee methods coz it'll give some accurate values. But predictGasPriceLegacy will also return valid values on post London blocks, but it'll be useless for transaction fee prediction since there's no way to get proportion to split that sum. So this comment is awareing method user, that this method is not designed to be used in post London blocks.

Except, not all chains are post-london. and not all transactions post-london are EIP-1559. My point is that code for a non-EIP-1159 transaction should be able to call predictGasPriceLegacy() and get an appropriate result, regardless if this is pre or post London fork, as they have no concept of baseFee or tip and should not have to add code to try to derive gasPrice from baseFee and Tip... the Oracle should do that.

predictGasPriceLegacy() function would never be called by code for an EIP-1159 transaction, instead code for a EIP-1159 transaction would call the appropriate predictTip() and predictBaseFee() functions.

@yaroslavyaroslav
Copy link
Collaborator Author

yaroslavyaroslav commented Apr 8, 2022

My point is that code for a non-EIP-1159 transaction should be able to call predictGasPriceLegacy() and get an appropriate result, regardless if this is pre or post London fork, as they have no concept of baseFee or tip and should not have to add code to try to derive gasPrice from baseFee and Tip... the Oracle should do that.

predictGasPriceLegacy() function would never be called by code for an EIP-1159 transaction, instead code for a EIP-1159 transaction would call the appropriate predictTip() and predictBaseFee() functions.

I read this two paragraph as contradicting each other, and i'm struggling to get your point. Let me explain my view more structured.

My assumptions:

  1. Each EIP-1559 transaction/block (because some of that four methods reads blocks and some reads transactions) (regardless of what the name of the fork it is) have all three properties, which is baseFee, maxPriorityFee and gasPrice.
  2. Pre EIP-1559 transaction/block have only gasPrice property.

So if this is true, we'll have follow Oracle methods results matrix:

Method name Pre EIP-1559 behavior EIP-1559 behavior
predictBaseFee nil correct value
predictTip nil correct value
predictBothFees nil correct value
predictGasPriceLegacy correct value correct value

So as shown above in the fourth row, predictGasPriceLegacy will all the time return correct value, coz it'll always (i mean until some further breaking changes with blocks properties happened) will have properties to rely on co calculate its result.

So as i see we're talking about doc comment to the legacy method, where i mention to a method user, that he should not use it in EIP-1559 blocks, since it'll be error prone, coz this method would return some valid values, but that values are useless within EIP-1559 transaction/block environment.

I see your point as give user possibility to call predictGasPriceLegacy method in post london blocks in a main net and to call it in all other networks, so to remove any connection to the forks versioning. And if i get your point right — it's already is. And more than that the comment that we discussing is pointing it to the user, that they using this method on they own risk, and they wouldn't get nil if they use this method in unappropriate environment (e.g. chain version)

@mloit
Copy link
Contributor

mloit commented Apr 8, 2022

Your table clears things up, and is the desired mode I have been asking for. However it goes against the mention of "Should be used only on pre EIP-1559 blocks." comment in your PR description that started this conversation. That mention suggests that the predictGasPriceLegacy() can only be used on pre-london fork blocks, and would not work on blocks after.

And to be clear, I was never suggesting that someone would use predictGasPriceLegacy() when working with an EIP-1559 transaction. That would be an implementation error on the user/developers part. That would be the same as trying to set maxFeePerGas on a legacy transaction, it just doesn't work.

@mloit
Copy link
Contributor

mloit commented Apr 8, 2022

I read this two paragraph as contradicting each other, and i'm struggling to get your point. Let me explain my view more structured.

They are not contradictory. Perhaps you think that everything after the London fork is EIP-1159? If so that is not the case. The London fork only adds new transaction types, it does not remove or disallow the other types.

  • Only EIP-1159 transactions use predictTip() and predictBaseFee()and never predictGasPriceLegacy()
  • All other types (legacy and EIP-2930) would use predictGasPriceLegacy(), and never predictTip() and predictBaseFee().

Pre-London we can only create legacy transactions. Post London we can create EIP-1159, EIP-2930, and legacy transactions. With only EIP-1159 using the new fee structure

@mloit
Copy link
Contributor

mloit commented Apr 8, 2022

My assumptions:
1) Each EIP-1559 transaction/block (because some of that four methods reads blocks and some reads transactions) (regardless of what the name of the fork it is) have all three properties, which is baseFee, maxPriorityFee and gasPrice.
2) Pre EIP-1559 transaction/block have only gasPrice property.

Perhaps true at the block level, but not true at the transaction level.
All blocks post London will be EIP-1159 blocks and contain all 3 transaction types. Only EIP-1159 transactions have maxPriorityFeePerGas, and maxFeePerGas, and all other transaction types only have gasPrice

@yaroslavyaroslav
Copy link
Collaborator Author

Pre-London we can only create legacy transactions. Post London we can create EIP-1159, EIP-2930, and legacy transactions. With only EIP-1159 using the new fee structure

Now i see, so i'll change that comment than, to not use that method on transaction instead of block.

- Add method `decodeHex` for `Arrays<T> where T: DecodableFromHex`.
- Add feeHistory request.
- Add public feeHistory API call
- Move GasOracle from `Web3` scope to `web3.Eth` scope (watch out case of first "w")
- Change Oracle struct to work with feeHistory API call.
- Refactor `JSONRPCmethod` to make it more explicit.
- Refactor `InfuraWebsockerMethod` with a styling.
- Rename decodeHex method to swift name convention.
- Add FeeHistory struct initializer.
- Now it works with feeHistory JRON RPC Ethereum call
- Now all method returns percentiles, which user set on object init.
- Implement generic decodeHex methods for T: DecodableFromHex, [T], [[T]].
- Make RPC API `feeHistory` call internal.
- Reimplement `tipFeePercentiles` to return array of percentiles of last tips.
- Reimplement `baseFeePercentiles` to return array of percentiles of last baseFee.
- Rename `bothFeesPercentiles` to return tuple of arrays of percentiles of last both tips and baseFee.
- `JSONRPCresponse` now can decode `Web3.Oracle.FeeHistory`
- `JSONRPCparams` now can encode `[Double]`
- Add `OracleTests`
   - `testPretictBaseFee`
   - `testPredictTip`
Copy link
Contributor

@mloit mloit left a comment

Choose a reason for hiding this comment

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

Just some comments on the changes in Decodable (and by extension Encodable). I think those changes are best left to a separate PR, as they are not specifically needed to implement gasPrediction.

Sources/web3swift/Convenience/Decodable+Extensions.swift Outdated Show resolved Hide resolved
guard contains(key),
try decodeNil(forKey: key) == false else { return nil }
return try decode(type, forKey: key)
/// - Parameter type: Array of a generic type `T` wich conforms to `DecodableFromHex` protocol
Copy link
Contributor

Choose a reason for hiding this comment

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

By removing the 'xxxIfPResent' variants you have removed the ability to distinguish between when an allowed optional was encountered, and when an actual decoding error occurred.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will it affect you that Array<Any> comes to [T] where T: HexDecodable

Copy link
Contributor

Choose a reason for hiding this comment

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

That should be fine. This comment was more about the xxxIfPresent variants being removed, than this one specific case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean that they will have such types either. But anyway i've realized that changing type of any of these public methods are going against versioning rules, so i'll bring them back, but mark as deprecated (including xxxIfPresent with Array<Any> types), and my PR will just include new generic methods here, and some light refactoring in JSON RPC encoder enums

- Update percentile arrays method.
- fix JSONRPCmethod validation bug
-
- Add `BlockNumber` enum to enable strong typing in block number management.
- Full test coverage of GasOracle public methods.
- Clean GasOracle class.
@yaroslavyaroslav yaroslavyaroslav marked this pull request as ready for review April 13, 2022 17:49
@yaroslavyaroslav yaroslavyaroslav merged commit f03f816 into develop Apr 14, 2022
@yaroslavyaroslav yaroslavyaroslav deleted the feature/gas-prediction branch April 15, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants