-
Notifications
You must be signed in to change notification settings - Fork 123
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
⛽️ Use the default gas cost params when lookup predicate cost #759
Conversation
WalkthroughThe changes involve a modification to the Changes
Possibly related issues
Poem
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #759 +/- ##
==========================================
+ Coverage 54.86% 54.89% +0.03%
==========================================
Files 73 73
Lines 2938 2940 +2
==========================================
+ Hits 1612 1614 +2
Misses 1230 1230
Partials 96 96
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/logic/keeper/interpreter.go (1)
Line range hint
209-216
: Effective reuse ofnonNilNorZeroOrDefaultUint64
functionThe
nonNilNorZeroOrDefaultUint64
function is well-utilized in the new context of determining the default predicate cost. Its implementation correctly handles nil and zero values, which is crucial for the robustness of the gas metering system.Consider renaming the function to
getValueOrDefault
orresolveDefaultValue
for improved clarity, as it now serves a more general purpose in the codebase. This would make its role more immediately apparent to other developers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- x/logic/keeper/interpreter.go (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: ccamel PR: axone-protocol/axoned#756 File: x/logic/keeper/interpreter.go:197-200 Timestamp: 2024-09-25T07:11:26.077Z Learning: Unregistered predicates are user-defined and should have a default gas consumption applied, referencing the module's `default_predicate_cost` configuration value instead of a hard-coded constant.
x/logic/keeper/interpreter.go (1)
Learnt from: ccamel PR: axone-protocol/axoned#756 File: x/logic/keeper/interpreter.go:197-200 Timestamp: 2024-09-25T07:11:26.077Z Learning: Unregistered predicates are user-defined and should have a default gas consumption applied, referencing the module's `default_predicate_cost` configuration value instead of a hard-coded constant.
🔇 Additional comments (2)
x/logic/keeper/interpreter.go (2)
176-178
: Improved robustness in predicate cost calculationThe changes to the
cost
calculation in thegasMeterHookFn
function are well-implemented and align with the PR objectives. By usingnonNilNorZeroOrDefaultUint64
, the code now handles cases wheregasPolicy.DefaultPredicateCost
might be nil or zero, falling back to thedefaultPredicateCost
constant. This improvement ensures that there's always a valid cost value, enhancing the robustness of the gas metering system.The modification also addresses the previous learning about using the module's configuration value for unregistered predicates, which is a good practice for maintaining consistency and flexibility in the system.
176-178
: Summary: Effective implementation of default predicate cost handlingThe changes in this PR are focused and effective. They successfully implement the requirement to maintain a default predicate cost when the module's parameters are nil or zero. This enhancement improves the robustness of the gas metering system and aligns well with previous feedback about handling unregistered predicates.
The implementation is clean, uses existing utility functions effectively, and doesn't introduce unnecessary complexity. It's a good example of making a targeted improvement that enhances the overall reliability of the system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This pull request includes a change to the
x/logic/keeper/interpreter.go
file to improve the handling of default predicate costs in thegasMeterHookFn
function following this issue #757 and since implementation of hooks in interpreter (#756).Keep the
defaultPredicateCost
const used to be the final default value in case no default cost set (nil or zero value) on module's params.Summary by CodeRabbit
New Features
Bug Fixes