Improve ErrorOutOfGas handling in predicate execution
#694
Conversation
WalkthroughThe changes across various files focus on enhancing error handling related to gas management and bank operations, improving test case clarity, and removing obsolete functions. These updates aim to boost the reliability and maintainability of the codebase while providing more context in error messages. Changes
Sequence Diagram(s)Not applicable for these changes as they are too varied and do not introduce new features or control flow modifications. 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## main #694 +/- ##
==========================================
- Coverage 54.35% 54.23% -0.12%
==========================================
Files 74 74
Lines 2896 2906 +10
==========================================
+ Hits 1574 1576 +2
- Misses 1226 1234 +8
Partials 96 96
|
bdeneux
force-pushed
the
fix/out-of-gas-catch
branch
2 times, most recently
from
9d478fe to
f1c1abd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
x/logic/keeper/interpreter.go (1)
92-95: Consider initializingenvexplicitly.While
envis used later in the function, it might be beneficial to explicitly initialize it to avoid potential issues.- var env *engine.Env + env := engine.NewEnv()
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- x/logic/keeper/grpc_query_ask_test.go (3 hunks)
- x/logic/keeper/interpreter.go (4 hunks)
Additional context used
GitHub Check: codecov/patch
x/logic/keeper/interpreter.go
[warning] 107-107: x/logic/keeper/interpreter.go#L107
Added line #L107 was not covered by tests
[warning] 244-244: x/logic/keeper/interpreter.go#L244
Added line #L244 was not covered by tests
[warning] 257-257: x/logic/keeper/interpreter.go#L257
Added line #L257 was not covered by tests
Additional comments not posted (12)
x/logic/keeper/interpreter.go (9)
33-33: LGTM!The new constant
defaultEnvCapis a useful addition for setting a default environment capacity.
86-90: Ensure proper error handling for parsing queries.The error handling for parsing queries seems appropriate. Ensure that all possible parsing errors are covered.
96-101: LGTM!The logic for handling solution limits and appending environments is sound.
103-108: Ensure comprehensive test coverage forenvsToResults.The function
envsToResultsis crucial for processing results. Ensure that it is thoroughly tested.Tools
GitHub Check: codecov/patch
[warning] 107-107: x/logic/keeper/interpreter.go#L107
Added line #L107 was not covered by tests
125-129: LGTM!The final result struct construction and return logic are appropriate.
226-230: LGTM!The
parsedVarsToVarsfunction correctly maps parsed variables to their string representations.
232-251: Ensure comprehensive test coverage forenvsToResults.The function
envsToResultsis crucial for processing results. Ensure that it is thoroughly tested.Tools
GitHub Check: codecov/patch
[warning] 244-244: x/logic/keeper/interpreter.go#L244
Added line #L244 was not covered by tests
253-265: LGTM!The
scanExpressionfunction correctly scans expressions and returns substitutions.Tools
GitHub Check: codecov/patch
[warning] 257-257: x/logic/keeper/interpreter.go#L257
Added line #L257 was not covered by tests
267-272: LGTM!The
isBoundfunction correctly checks if a parsed variable is bound in the given environment.x/logic/keeper/grpc_query_ask_test.go (3)
23-23: LGTM!The new import
bankypesis necessary for the new test case for a bank query.
142-149: LGTM!The new test case for a bank query correctly tests the handling of bank balances and gas consumption.
338-342: LGTM!The updated mock interaction for
bankKeepercorrectly simulates gas consumption for theGetAccountsBalancesmethod.
|
@bdeneux Thanks! I wonder why we've integrated the out of gas as a prolog error, it creates a discrepancy in the way the @ccamel I'd be really interested in your insights on that, I may be missing something :) |
bdeneux
force-pushed
the
fix/out-of-gas-catch
branch
from
f1c1abd to
fedbafb
Compare
bdeneux
force-pushed
the
fix/out-of-gas-catch
branch
from
fedbafb to
fdc9db9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
x/logic/keeper/interpreter.go (1)
112-116: Ensure test coverage for new error handling logic.The changes improve error handling by specifically addressing out-of-gas errors. However, the new error handling logic is not covered by tests. Please add test cases to cover this new logic.
Analysis chain
Ensure test coverage for new error handling logic.
The changes improve error handling by specifically addressing out-of-gas errors. However, the new error handling logic is not covered by tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure test coverage for new error handling logic. # Test: Search for test cases covering the new error handling logic. Expect: Test cases covering the new error handling logic. rg --type go -A 5 $'storetypes.ErrorOutOfGas'Length of output: 874
Tools
GitHub Check: codecov/patch
[warning] 115-116: x/logic/keeper/interpreter.go#L115-L116
Added lines #L115 - L116 were not covered by tests
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- x/logic/keeper/grpc_query_ask_test.go (3 hunks)
- x/logic/keeper/interpreter.go (2 hunks)
- x/logic/prolog/error.go (3 hunks)
- x/logic/util/prolog.go (2 hunks)
Additional context used
GitHub Check: codecov/patch
x/logic/util/prolog.go
[warning] 57-60: x/logic/util/prolog.go#L57-L60
Added lines #L57 - L60 were not covered by testsx/logic/keeper/interpreter.go
[warning] 115-116: x/logic/keeper/interpreter.go#L115-L116
Added lines #L115 - L116 were not covered by tests
Additional comments not posted (13)
x/logic/util/prolog.go (4)
8-8: Import addition approved.The import for
errorsmodis necessary for wrapping errors with additional context.
10-10: Import addition approved.The import for
sdkis necessary for accessing SDK-specific types and functions.
11-11: Import addition approved.The import for
sdkerrorsis necessary for handling SDK-specific errors.
57-61: Ensure test coverage for new error handling logic.The changes improve error handling by specifically addressing out-of-gas errors. However, the new error handling logic is not covered by tests.
Tools
GitHub Check: codecov/patch
[warning] 57-60: x/logic/util/prolog.go#L57-L60
Added lines #L57 - L60 were not covered by testsx/logic/prolog/error.go (3)
5-5: Import removal approved.The import for
cosmossdk.io/store/typesis no longer necessary as the related components are removed.
5-5: Function removal approved.The function
AtomResourceGasis no longer necessary as the related components are removed.
5-5: Function removal approved.The function
ResourceGasis no longer necessary as the related components are removed.x/logic/keeper/interpreter.go (1)
10-10: Import addition approved.The import for
sdkerrorsis necessary for handling SDK-specific errors.x/logic/keeper/grpc_query_ask_test.go (5)
20-20: Import addition approved.The import for
sdkis necessary for accessing SDK-specific types and functions.
23-23: Import addition approved.The import for
bankypesis necessary for accessing bank-specific types and functions.
142-146: Test case addition approved.The new test case covers the error handling logic for bank balances and gas consumption. Ensure that the test case passes successfully.
153-153: Test case addition approved.The new test case covers the error handling logic for block height and gas consumption. Ensure that the test case passes successfully.
332-336: Test case addition approved.The new test case logic for gas consumption related to bank operations is necessary for covering the new error handling logic. Ensure that the test case passes successfully.
bdeneux
force-pushed
the
fix/out-of-gas-catch
branch
from
594c1c4 to
f117c7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- x/logic/keeper/interpreter.go (2 hunks)
- x/logic/util/prolog.go (2 hunks)
Additional context used
GitHub Check: codecov/patch
x/logic/util/prolog.go
[warning] 56-62: x/logic/util/prolog.go#L56-L62
Added lines #L56 - L62 were not covered by testsx/logic/keeper/interpreter.go
[warning] 114-115: x/logic/keeper/interpreter.go#L114-L115
Added lines #L114 - L115 were not covered by tests
Additional comments not posted (6)
x/logic/util/prolog.go (4)
12-12: LGTM! Import forerrorsmodis necessary.The import
errorsmodfromcosmossdk.io/errorsis correctly added for enhanced error handling.
15-15: LGTM! Import forsdkis necessary.The import
sdkfromgithub.com/cosmos/cosmos-sdk/typesis correctly added for using SDK types.
16-16: LGTM! Import forsdkerrorsis necessary.The import
sdkerrorsfromgithub.com/cosmos/cosmos-sdk/types/errorsis correctly added for handling SDK-specific errors.
56-63: Enhanced error handling forsdkerrors.ErrOutOfGas.The error handling logic is improved to handle out-of-gas scenarios more robustly using a switch statement.
However, ensure that these changes are covered by tests.
Tools
GitHub Check: codecov/patch
[warning] 56-62: x/logic/util/prolog.go#L56-L62
Added lines #L56 - L62 were not covered by testsx/logic/keeper/interpreter.go (2)
19-19: LGTM! Import forsdkerrorsis necessary.The import
sdkerrorsfromgithub.com/cosmos/cosmos-sdk/types/errorsis correctly added for handling SDK-specific errors.
111-115: Enhanced error handling forstoretypes.ErrorOutOfGas.The error handling logic is improved to handle out-of-gas scenarios more robustly using a switch statement.
However, ensure that these changes are covered by tests.
Tools
GitHub Check: codecov/patch
[warning] 114-115: x/logic/keeper/interpreter.go#L114-L115
Added lines #L114 - L115 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (1)
x/logic/util/prolog.go (1)
Line range hint
75-98:
Add test coverage for converting environments to results.The conversion of environments to results is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- x/logic/keeper/grpc_query_ask_test.go (3 hunks)
- x/logic/keeper/interpreter.go (2 hunks)
- x/logic/util/prolog.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/logic/keeper/grpc_query_ask_test.go
Additional context used
GitHub Check: codecov/patch
x/logic/util/prolog.go
[warning] 31-31: x/logic/util/prolog.go#L31
Added line #L31 was not covered by tests
[warning] 49-49: x/logic/util/prolog.go#L49
Added line #L49 was not covered by tests
[warning] 55-55: x/logic/util/prolog.go#L55
Added line #L55 was not covered by tests
[warning] 57-65: x/logic/util/prolog.go#L57-L65
Added lines #L57 - L65 were not covered by testsx/logic/keeper/interpreter.go
[warning] 115-116: x/logic/keeper/interpreter.go#L115-L116
Added lines #L115 - L116 were not covered by tests
Additional comments not posted (2)
x/logic/util/prolog.go (1)
Line range hint
71-73:
LGTM!The function correctly maps parsed variables to their string names.
x/logic/keeper/interpreter.go (1)
Line range hint
76-78:
LGTM!The function correctly delegates the query execution to
util.QueryInterpreter.
Fixe #692, see issue description for complete details.
Note: I was forced to moveQueryInterpreterutility method into the keeper since using the commonResourceGaspredicate error coming from keeper package lead to a cycle import betweenutilsandprologpackages.EDIT: Now all
OutOfGaserrors that occurs in the interpreter or at predicate cost will return an GRPC error instead of a predicate prolog error.Summary by CodeRabbit
Bug Fixes
Refactor
ifstatements toswitchstatements for better code readability and maintainability.