feat: implement the gas estimation API #4257
Conversation
rach-id
requested review from
cmwaters and
ninabarbakadze
and removed request for
a team
| // We'll do a tolerance check for floating-point comparisons. | ||
| if math.Abs(got-tt.want) > 1e-9 { | ||
| t.Errorf("stdDev(%v) = %v, want %v", tt.gasPrices, got, tt.want) | ||
| } |
There was a problem hiding this comment.
this is chatgpt's doing and I think it's cool. we can remove if it looks complicated :D
📝 WalkthroughWalkthroughThis pull request introduces enhancements to the gas estimation functionality within the Celestia blockchain application. Key changes include the addition of a gRPC service for estimating gas prices and usage, the implementation of statistical methods for gas price calculations, and the creation of corresponding test cases. The Changes
Sequence DiagramsequenceDiagram
participant Client
participant GasEstimationService
participant Blockchain
Client->>GasEstimationService: EstimateGasPrice(request)
GasEstimationService->>Blockchain: Fetch recent transactions
Blockchain-->>GasEstimationService: Transaction data
GasEstimationService->>GasEstimationService: Calculate gas price
GasEstimationService-->>Client: Return estimated gas price
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
LGTM.
One suggestion is to use medians like was originally stated in the ADR (median of top 10%, median, median of bottom 10%).
If we use averages and z-scores, then we should confirm that the gas prices in historical blocks on Mainnet follow a normal distribution.
| // EstimationZScore is the z-score corresponding to 10% and 90% of the gas prices distribution. | ||
| // More information can be found in: https://en.wikipedia.org/wiki/Standard_normal_table#Cumulative_(less_than_Z) | ||
| const EstimationZScore = 1.28 |
There was a problem hiding this comment.
Before we use Z score, IMO we should verify that the gas prices in historical blocks form a normal distribution.
I tried to convey in the ADR that median of top 10% is likely easier and good enough so we don't need to use z scores.
There was a problem hiding this comment.
Before we use Z score, IMO we should verify that the gas prices in historical blocks form a normal distribution.
Having a normal distribution is optional in our case IMO since we don't need spot on values. There are three cases:
- a flat distribution: where the values will generally be close the to the mean which is fine
- a U distribution (I mean just the shape of the distribution forming an U or a W or similar): where we have two cases: either the low prices will be higher than 10%. Or the high prices will be lower than 90%. but I guess that's fine.
I tried to convey in the ADR that median of top 10% is likely easier and good enough so we don't need to use z scores.
got you 👍 I avoided implementing the percentile because it means implementing a bit of complicated math method and maintaining it and I wanted to avoid that. If we can find a library we can use, that has an open source license, I'll definitely change this implementation.
| @@ -1,15 +1,18 @@ | |||
| # Generated by buf. DO NOT EDIT. | |||
| version: v1 | |||
| version: v1beta1 | |||
There was a problem hiding this comment.
what motivated this change?
There was a problem hiding this comment.
this is buf.lock. It gets updated automatically when you run buf dep update and it updated this for some reason :D
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
Co-authored-by: Rootul P <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
app/grpc/gasestimation/gas_estimator_test.go (2)
82-84: Adjust tolerance in floating-point comparisonIn
TestStandardDeviation, the tolerance for comparing floating-point numbers is very strict:if math.Abs(got-tt.want) > 1e-9 { t.Errorf("stdDev(%v) = %v, want %v", tt.gasPrices, got, tt.want) }Consider increasing the tolerance to account for floating-point precision:
- if math.Abs(got-tt.want) > 1e-9 { + if math.Abs(got-tt.want) > 1e-7 {
130-136: Use appropriate assertion for floating-point equalityIn
TestEstimateGasPriceForTransactions,assert.Equalis used to compare floating-point numbers:assert.Equal(t, tt.want, got)Use
assert.InDeltato allow for minor differences:- assert.Equal(t, tt.want, got) + assert.InDelta(t, tt.want, got, 1e-7)app/test/gas_estimation_test.go (1)
126-127: Useassert.InDeltafor comparing floating-point valuesIn
TestEstimateGasPrice, floating-point numbers are compared using string formatting:assert.Equal(t, fmt.Sprintf("%.2f", tt.expectedGasPrice), fmt.Sprintf("%.2f", resp.EstimatedGasPrice))Replace with
assert.InDeltafor better precision handling:- assert.Equal(t, fmt.Sprintf("%.2f", tt.expectedGasPrice), fmt.Sprintf("%.2f", resp.EstimatedGasPrice)) + assert.InDelta(t, tt.expectedGasPrice, resp.EstimatedGasPrice, 1e-2)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/grpc/gasestimation/gas_estimator.pb.gois excluded by!**/*.pb.gobuf.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Makefile(1 hunks)app/app.go(2 hunks)app/grpc/gasestimation/gas_estimator.go(1 hunks)app/grpc/gasestimation/gas_estimator_test.go(1 hunks)app/test/gas_estimation_test.go(1 hunks)buf.yaml(1 hunks)docs/architecture/adr-023-gas-used-and-gas-price-estimation.md(1 hunks)proto/celestia/core/v1/gas_estimation/gas_estimator.proto(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/architecture/adr-023-gas-used-and-gas-price-estimation.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Summary
🔇 Additional comments (9)
app/grpc/gasestimation/gas_estimator.go (2)
173-189: Ensure proper handling of emptygasPricesin estimationThe function
estimateGasPriceForTransactionsdoes not explicitly handle the case wheregasPricesis empty. This could unintentionally return an estimated gas price of zero.Confirm whether returning zero is acceptable when
gasPricesis empty. If not, consider modifying the function to return the minimum gas price in such cases.
19-21: Reconsider use of Z-score without verifying distributionUsing Z-scores assumes that gas prices follow a normal distribution. If this assumption doesn't hold, the estimates might be inaccurate.
Consider verifying the distribution of gas prices. If they are not normally distributed, alternative methods like percentile calculations might be more appropriate.
app/test/gas_estimation_test.go (1)
168-169: Enhance test validity by avoiding circular dependenciesIn
TestEstimateGasUsed, the expected gas is obtained usingtxClient.EstimateGas, which relies on the same simulation logic as the method under test.To make the test more meaningful, compare the estimated gas against hard-coded expected values or use known benchmark transactions.
app/app.go (1)
9-9: LGTM! Clean integration of the gas estimation service.The changes correctly integrate the gas estimation service by adding the required import and registering the service with the gRPC query router.
Also applies to: 758-758
buf.yaml (1)
Line range hint
9-14: LGTM! Standard buf.build configuration.The build configuration correctly specifies the proto roots and excludes the standard Google protobuf definitions.
proto/celestia/core/v1/gas_estimation/gas_estimator.proto (4)
1-8: LGTM! Clean protobuf setup.The package declaration and imports are correctly structured.
9-28: LGTM! Well-documented service definition.The GasEstimator service is clearly defined with comprehensive documentation for both RPC methods.
30-46: Improve documentation clarity for priority levels.The documentation could be more precise about the statistical methods used for each priority level.
Apply this diff to improve the documentation:
-// - Medium Priority: The gas price is the mean of all gas prices from the last 5 blocks. -// - Low Priority: The gas price is the value at the end of the lowest 10% of gas prices from the last 5 blocks. +// - Medium Priority: The gas price is the mean of all gas prices from the last 5 blocks. +// - Low Priority: The gas price is the value at the end of the lowest 10% of gas prices from the last 5 blocks.
48-71: LGTM! Clear message definitions.The request and response message definitions are well-structured with appropriate field types.
| gasPrices := make([]float64, 0) | ||
| for _, tx := range txs { | ||
| var feeWithDenom string | ||
| for _, event := range tx.TxResult.Events { | ||
| if event.GetType() == "tx" { | ||
| for _, attr := range event.Attributes { | ||
| if string(attr.Key) == "fee" { | ||
| feeWithDenom = string(attr.Value) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if feeWithDenom == "" { | ||
| return nil, fmt.Errorf("couldn't find fee for transaction %s", tx.Hash) | ||
| } | ||
| feeWithoutDenom, found := strings.CutSuffix(feeWithDenom, appconsts.BondDenom) | ||
| if !found { | ||
| return nil, fmt.Errorf("couldn't find fee denom for transaction %s: %s", tx.Hash, feeWithDenom) | ||
| } | ||
| fee, err := strconv.ParseFloat(feeWithoutDenom, 64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("couldn't parse fee for transaction %s: %w", tx.Hash, err) | ||
| } | ||
| gasPrices = append(gasPrices, fee/float64(tx.TxResult.GasWanted)) | ||
| } | ||
| return gasPrices, nil | ||
| } |
There was a problem hiding this comment.
Prevent division by zero in gas price calculation
In the extractGasPriceFromTransactions function, there's a risk of dividing by zero if tx.TxResult.GasWanted is zero:
gasPrices = append(gasPrices, fee/float64(tx.TxResult.GasWanted))Add a check to prevent division by zero:
+ if tx.TxResult.GasWanted == 0 {
+ return nil, fmt.Errorf("GasWanted is zero for transaction %s", tx.Hash)
+ }
gasPrices = append(gasPrices, fee/float64(tx.TxResult.GasWanted))
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/grpc/gasestimation/gas_estimator.go (2)
87-89:⚠️ Potential issueFix error handling for UnmarshalBlobTx
The error check should be independent of
isBlob. IfUnmarshalBlobTxreturns an error, it should be handled regardless of the blob status.-if isBlob && err != nil { +if err != nil { return nil, err }
168-172:⚠️ Potential issueUpdate comments to match implementation
The comments describe percentile-based calculations, but the implementation uses mean and standard deviation.
-// - High Priority: The gas price is the price at the start of the top 10% of transactions' gas prices from the last five blocks. -// - Medium Priority: The gas price is the mean of all gas prices from the last five blocks. -// - Low Priority: The gas price is the value at the end of the lowest 10% of gas prices from the last five blocks. -// - Unspecified Priority (default): This is equivalent to the Medium priority, using the mean of all gas prices from the last five blocks. +// - High Priority: The gas price is the mean plus EstimationZScore times the standard deviation. +// - Medium Priority: The gas price is the mean of all gas prices from the last five blocks. +// - Low Priority: The gas price is the mean minus EstimationZScore times the standard deviation. +// - Unspecified Priority (default): Equivalent to Medium priority.
🧹 Nitpick comments (2)
app/grpc/gasestimation/gas_estimator.go (2)
61-61: Convert variable to constantThe
numberOfTransactionsPerPageshould be declared as a constant since it represents a fixed limit.-var numberOfTransactionsPerPage = 100 +const numberOfTransactionsPerPage = 100
108-163: Consider caching gas price estimatesThe
estimateGasPricemethod queries the last five blocks for every request. Consider implementing a caching mechanism with a short TTL (e.g., 1 block) to reduce RPC calls and improve performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/grpc/gasestimation/gas_estimator.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
app/grpc/gasestimation/gas_estimator.go (1)
217-219: LGTM! Good error handlingProper check for division by zero has been implemented.
## Overview Implements the gas estimation endpoint following: https://github.com/celestiaorg/CIPs/blob/main/cips/cip-18.md Is there an issue for this or I create one? --------- Co-authored-by: Rootul P <[email protected]> (cherry picked from commit 02f04c9)
## Overview Implements the gas estimation endpoint following: https://github.com/celestiaorg/CIPs/blob/main/cips/cip-18.md Is there an issue for this or I create one? --------- Co-authored-by: Rootul P <[email protected]> (cherry picked from commit 02f04c9)
## Overview Implements the gas estimation endpoint following: https://github.com/celestiaorg/CIPs/blob/main/cips/cip-18.md Is there an issue for this or I create one? --------- Co-authored-by: Rootul P <[email protected]> (cherry picked from commit 02f04c9)
Overview
Implements the gas estimation endpoint following: https://github.com/celestiaorg/CIPs/blob/main/cips/cip-18.md
Is there an issue for this or I create one?