refact(bls):implement gas calculation logic #325
Conversation
WalkthroughThe pull request updates the BLS precompile and its associated interfaces by standardizing naming conventions and refining method functionality. Changes include renaming parameters and return types to use camel case consistently, removing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
CodeRabbit Configuration File (
|
trestinlsd
requested review from
MaxMustermann2,
bwhour,
mikebraver,
TimmyExogenous and
leonz789
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
precompiles/bls/bls.go (1)
165-166: Simplified IsTransaction method signature and implementation.The method has been changed to always return
falsewithout requiring a method ID parameter. This is appropriate since none of the BLS methods are transactions (they're stateless precompile calls).However, the function's documentation still mentions "Available bls transactions are: - MethodVerify" which contradicts the implementation.
// IsTransaction checks if the given methodID corresponds to a transaction or query. // -// Available bls transactions are: -// - MethodVerify +// BLS precompiles are stateless and don't modify state, so this always returns false. func (Precompile) IsTransaction() bool { return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
precompiles/bls/bls.go(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
precompiles/bls/bls.go (2)
precompiles/bls/methods.go (10) (10)
p(25:56)p(59:95)p(97:116)p(118:136)p(138:165)MethodVerify(18:18)MethodFastAggregateVerify(16:16)MethodAggregatePubKeys(19:19)MethodAggregateSignatures(20:20)MethodAddTwoPubKeys(21:21)precompiles/bls/protocol_params.go (4) (4)
Bls12381PairingBaseGas(9:9)Bls12381PairingPerPairGas(10:10)Bls12381G1AddGas(5:5)Bls12381G2AddGas(7:7)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-unit-e2e
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
precompiles/bls/bls.go (6)
7-7: Added required import for gas calculation logic.The import of
math/bigpackage is correctly added to support the new gas calculation functions that use big integers for parsing input data.
32-45: Simplified precompile initialization by removing unused baseGas parameter.The removal of the
baseGasparameter fromNewPrecompileis appropriate since gas calculations are now dynamically determined in theRequiredGasmethod. This change simplifies the interface and eliminates an unused parameter.
55-83: Implemented dynamic gas calculation based on method selectors.The gas calculation logic now properly determines gas costs based on the specific BLS operation being performed, following a similar pattern to other Ethereum precompiles. This is a significant improvement from the previous fixed gas cost approach.
The implementation correctly uses
p.MethodByIdto identify the method and applies appropriate gas costs for each operation type, addressing the previous review comments.
86-102: Implemented specialized gas calculation for FastAggregateVerify.The function correctly calculates gas based on the number of public keys being verified:
- Extracts the public keys offset from the input data
- Determines the number of keys (m)
- Calculates gas as: (m-1)*G1AddGas + PairingGas
The defensive input length checks help prevent potential panics when parsing malformed inputs.
105-121: Added generic aggregation gas calculation logic.This helper function is well-designed to:
- Extract the offset of the array from the input
- Determine the number of elements (n)
- Calculate gas as (n-1) * gasPerOp
The function includes appropriate input length validation and handles the case where n < 1, providing robust error handling for edge cases.
144-145: Standardized method naming convention.The method names have been updated from
AggregatePubkeystoAggregatePubKeysand fromAddTwoPubkeystoAddTwoPubKeys, using consistent camelCase with capital "K" in "Keys". This improves naming consistency across the codebase.Also applies to: 148-149
Description
Summary by CodeRabbit
Summary by CodeRabbit