🧠 Use our own axone/prolog fork in logic module
#703
Conversation
WalkthroughThe recent changes enhance the functionality and configurability of a Go project, particularly within the Prolog interpreter and its associated components. Key updates include the introduction of a maximum variable limit, improvements in error handling, and the adoption of ordered maps for better predicate management. These enhancements streamline resource management and improve testing capabilities, thereby refining overall performance and robustness. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Interpreter
participant ErrorHandler
User->>Interpreter: Execute query with variables
Interpreter->>ErrorHandler: Check for variable limits
ErrorHandler-->>Interpreter: Return error if limit exceeded
Interpreter-->>User: Return results or error
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 (
|
bdeneux
force-pushed
the
chore/axone-prolog-interpreter
branch
from
9d94b01
to
90f1fa6
Compare
bdeneux
force-pushed
the
chore/axone-prolog-interpreter
branch
3 times, most recently
from
d2436f2
to
dce2242
Compare
|
@amimart This PR is now ready until we release axone-protocol/prolog 😇 |
bdeneux
force-pushed
the
chore/axone-prolog-interpreter
branch
from
dce2242
to
8b9b329
Compare
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## main #703 +/- ##
==========================================
+ Coverage 54.37% 54.42% +0.05%
==========================================
Files 73 73
Lines 2893 2901 +8
==========================================
+ Hits 1573 1579 +6
- Misses 1226 1228 +2
Partials 94 94
|
bdeneux
force-pushed
the
chore/axone-prolog-interpreter
branch
from
2f558ae
to
7e1d7ae
Compare
bdeneux
force-pushed
the
chore/axone-prolog-interpreter
branch
from
7e1d7ae
to
87271fb
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, codebase verification and nitpick comments (1)
proto/logic/v1beta2/params.proto (1)
63-64: Fix typo in comment.The comment for
max_variablescontains a minor typo: "create" should be "created".- // max_variables specifies the maximum number of variables that can be create by the interpreter. + // max_variables specifies the maximum number of variables that can be created by the interpreter.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (10)
docs/predicate/bech32_address_2.mdis excluded by!docs/predicate/**/*.mddocs/predicate/block_height_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/block_time_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/consult_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/current_output_1.mdis excluded by!docs/predicate/**/*.mddocs/predicate/open_3.mdis excluded by!docs/predicate/**/*.mddocs/predicate/open_4.mdis excluded by!docs/predicate/**/*.mddocs/proto/logic.mdis excluded by!docs/proto/**/*.mdgo.sumis excluded by!**/*.sumx/logic/types/params.pb.gois excluded by!**/*.pb.go
Files selected for processing (18)
- go.mod (4 hunks)
- proto/logic/v1beta2/params.proto (1 hunks)
- x/logic/interpreter/interpreter.go (2 hunks)
- x/logic/interpreter/registry.go (2 hunks)
- x/logic/keeper/features/bech32_address_2.feature (17 hunks)
- x/logic/keeper/features/block_height_1.feature (2 hunks)
- x/logic/keeper/features/block_time_1.feature (2 hunks)
- x/logic/keeper/features/consult_1.feature (3 hunks)
- x/logic/keeper/features/current_output_1.feature (5 hunks)
- x/logic/keeper/features/open_3.feature (1 hunks)
- x/logic/keeper/features/open_4.feature (11 hunks)
- x/logic/keeper/grpc_query_ask_test.go (3 hunks)
- x/logic/keeper/grpc_query_params_test.go (1 hunks)
- x/logic/keeper/interpreter.go (1 hunks)
- x/logic/prolog/tuple.go (1 hunks)
- x/logic/types/params.go (3 hunks)
- x/logic/types/params_test.go (1 hunks)
- x/logic/util/prolog.go (1 hunks)
Additional context used
Learnings (1)
x/logic/util/prolog.go (1)
Learnt from: bdeneux PR: axone-protocol/axoned#694 File: x/logic/util/prolog.go:31-31 Timestamp: 2024-07-22T09:40:50.049Z Learning: The error handling for parsing terms in the `QueryInterpreter` function is covered by tests in `grpc_query_ask_test.go` around line 230.
GitHub Check: codecov/patch
x/logic/prolog/tuple.go
[warning] 7-7: x/logic/prolog/tuple.go#L7
Added line #L7 was not covered by testsx/logic/util/prolog.go
[warning] 57-57: x/logic/util/prolog.go#L57
Added line #L57 was not covered by tests
[warning] 61-62: x/logic/util/prolog.go#L61-L62
Added lines #L61 - L62 were not covered by tests
[warning] 68-68: x/logic/util/prolog.go#L68
Added line #L68 was not covered by tests
Additional comments not posted (43)
x/logic/prolog/tuple.go (1)
7-7: Verify the impact of changingengine.Atom(0)toengine.Atom("").The change alters the internal representation of the tuple. Ensure that this modification does not introduce any unintended side effects in tuple handling or unification within the Prolog engine.
Tools
GitHub Check: codecov/patch
[warning] 7-7: x/logic/prolog/tuple.go#L7
Added line #L7 was not covered by testsx/logic/keeper/features/open_3.feature (1)
34-34: Verify the accuracy of the updatedgas_usedvalue.The
gas_usedvalue has been updated from3877to4141. Ensure that this new value accurately reflects the expected gas usage for the operation.x/logic/keeper/features/block_height_1.feature (2)
19-19: Verify the accuracy of the updatedgas_usedvalue.The
gas_usedvalue has been updated from3876to4140. Ensure that this new value accurately reflects the expected gas usage for the operation.
46-46: Verify the accuracy of the updatedgas_usedvalue.The
gas_usedvalue has been updated from3877to4141. Ensure that this new value accurately reflects the expected gas usage for the operation.x/logic/keeper/features/block_time_1.feature (2)
19-19: Verify the updatedgas_usedvalue.The
gas_usedvalue has been updated from3876to4140. Ensure that this change is consistent with the underlying calculations and logic.
47-47: Verify the updatedgas_usedvalue.The
gas_usedvalue has been updated from3877to4141. Ensure that this change is consistent with the underlying calculations and logic.x/logic/types/params_test.go (1)
42-42: Verify the new parametertypes.WithMaxVariables(math.NewUint(5)).The new parameter
types.WithMaxVariables(math.NewUint(5))has been added to the test case. Ensure that this change is consistent with the intended functionality and does not introduce any unintended side effects.x/logic/interpreter/interpreter.go (3)
11-12: Verify the necessity of the new import.The import statement for
cosmossdk.io/mathhas been added. Ensure that this import is necessary and correctly used in the file.
73-82: Verify the new functionWithMaxVariables.The function
WithMaxVariableshas been added to set a maximum number of variables for the interpreter. Ensure that this function is correctly implemented and does not introduce any unintended side effects.
88-96: Verify the changes to theNewfunction.The
Newfunction has been modified to useprolog.NewEmpty()for creating an instance of the interpreter. Ensure that this change is consistent with the intended functionality and does not introduce any unintended side effects.x/logic/keeper/grpc_query_params_test.go (1)
46-46: LGTM! Ensure adequate test coverage.The addition of
types.WithMaxVariables(math.NewUint(5))enhances the test cases by specifying a maximum number of variables. Ensure that all edge cases related to variable limits are covered in the tests.x/logic/util/prolog.go (3)
57-57: LGTM! Ensure test coverage for new error handling.The introduction of
panicErrand the additional error handling logic forengine.ErrMaxVariablesimprove the robustness of theQueryInterpreterfunction. Ensure that the new error handling paths are covered by tests.Tools
GitHub Check: codecov/patch
[warning] 57-57: x/logic/util/prolog.go#L57
Added line #L57 was not covered by tests
61-62: LGTM! Ensure test coverage for new error handling.The new case in the switch statement for
panicErrenhances the specificity of error reporting by handlingengine.ErrMaxVariables. Ensure that the new error handling paths are covered by tests.Tools
GitHub Check: codecov/patch
[warning] 61-62: x/logic/util/prolog.go#L61-L62
Added lines #L61 - L62 were not covered by tests
68-68: LGTM! Ensure test coverage for error logging.The modification to the default case in the switch statement ensures that all errors are consistently recorded, improving debugging and error tracking. Ensure that the error logging paths are covered by tests.
Tools
GitHub Check: codecov/patch
[warning] 68-68: x/logic/util/prolog.go#L68
Added line #L68 was not covered by testsx/logic/types/params.go (3)
20-20: LGTM!The introduction of
DefaultMaxVariablesinitialized to 100,000 provides a sensible default limit for the maximum number of variables, enhancing the configurability of the interpreter.
150-155: LGTM!The new function
WithMaxVariablesenhances the flexibility of theLimitsstructure by allowing users to specify a custom maximum number of variables.
172-175: LGTM!The update to the
NewLimitsfunction ensures that a sensible default forMaxVariablesis applied if it is not explicitly set, enhancing the robustness of theLimitsstructure.x/logic/keeper/features/current_output_1.feature (5)
31-31: Update ingas_usedvalue.The
gas_usedvalue has been updated from3976to4240. Ensure that this change aligns with the expected performance metrics.
69-69: Update ingas_usedvalue.The
gas_usedvalue has been updated from4010to4274. Ensure that this change aligns with the expected performance metrics.
107-107: Update ingas_usedvalue.The
gas_usedvalue has been updated from3976to4240. Ensure that this change aligns with the expected performance metrics.
148-148: Update ingas_usedvalue.The
gas_usedvalue has been updated from3990to4254. Ensure that this change aligns with the expected performance metrics.
179-179: Update ingas_usedvalue.The
gas_usedvalue has been updated from4261to4525. Ensure that this change aligns with the expected performance metrics.x/logic/keeper/interpreter.go (1)
136-136: Introduction ofWithMaxVariablesparameter.The
WithMaxVariables(limits.MaxVariables)parameter has been added to thenewInterpreterfunction to enhance the functionality of the interpreter by allowing it to accept a maximum variable limit. Ensure that this change aligns with the expected performance improvements and resource constraints.x/logic/keeper/features/consult_1.feature (3)
37-37: Update ingas_usedvalue.The
gas_usedvalue has been updated from3878to4142. Ensure that this change aligns with the expected performance metrics.
93-93: Update ingas_usedvalue.The
gas_usedvalue has been updated from3877to4141. Ensure that this change aligns with the expected performance metrics.
147-147: Update ingas_usedvalue.The
gas_usedvalue has been updated from3877to4141. Ensure that this change aligns with the expected performance metrics.x/logic/interpreter/registry.go (2)
16-123: LGTM!The transition to an ordered map and the initialization of the
registryvariable with predicate key-value pairs are appropriate.
128-131: LGTM!The changes to the
RegistryNamesfunction ensure that the list of predicate names maintains the same order as they were added to the registry.x/logic/keeper/features/open_4.feature (1)
46-46: LGTM!The updates to the
gas_usedparameter reflect necessary adjustments to the expected results.Also applies to: 89-89, 130-130, 152-152, 173-173, 194-194, 215-215, 235-235, 254-254, 273-273, 292-292
x/logic/keeper/features/bech32_address_2.feature (6)
19-19: Verify the updatedgas_usedvalue.The
gas_usedvalue has been updated from3876to4140. Ensure this change reflects the updated computational cost accurately.
42-42: Verify the updatedgas_usedvalue.The
gas_usedvalue has been updated from3876to4140. Ensure this change reflects the updated computational cost accurately.
66-66: Verify the updatedgas_usedvalue.The
gas_usedvalue has been updated from3876to4140. Ensure this change reflects the updated computational cost accurately.
87-87: Verify the updatedgas_usedvalue.The
gas_usedvalue has been updated from3876to4140. Ensure this change reflects the updated computational cost accurately.
112-112: Verify the updatedgas_usedvalue.The
gas_usedvalue has been updated from3876to4140. Ensure this change reflects the updated computational cost accurately.
133-133: Verify the updatedgas_usedvalue.The
gas_usedvalue has been updated from3876to4140. Ensure this change reflects the updated computational cost accurately.x/logic/keeper/grpc_query_ask_test.go (3)
43-43: LGTM! The addition ofmaxVariablesenhances the test cases.The new variable
maxVariablesis added to the test cases struct, allowing for better control over resource limits.
147-147: Verify the updated expected error message formaxGas.The expected error message has been updated to reflect new gas limit values. Ensure this change accurately reflects the updated resource consumption calculations.
157-160: LGTM! The new test case formaxVariablesenhances test coverage.The new test case checks for the condition where the maximum number of variables is exceeded, improving the robustness of the test suite.
go.mod (5)
57-57: LGTM! The new dependencies enhance the project's capabilities.The following dependencies have been added:
github.com/wk8/go-ordered-map/v2 v2.1.8github.com/bahlo/generic-list-go v0.2.0github.com/buger/jsonparser v1.1.1github.com/cockroachdb/apd/v3 v3.2.1github.com/mailru/easyjson v0.7.7
79-79: LGTM! The custom fork replacement addresses specific needs.The
ichiban/prologmodule is replaced with theaxone-protocol/prologfork, enhancing functionality and addressing specific issues.
104-104: LGTM! The new indirect dependency is appropriate.The following indirect dependency has been added:
github.com/bahlo/generic-list-go v0.2.0
112-112: LGTM! The new indirect dependency is appropriate.The following indirect dependency has been added:
github.com/buger/jsonparser v1.1.1
120-120: LGTM! The new indirect dependency is appropriate.The following indirect dependency has been added:
github.com/cockroachdb/apd/v3 v3.2.1
📝 Description
This PR replace ichiban/prolog module with our own fork axone-protocol/prolog. By doing this, since it's contains breaking changes, we need to align current implementation with this new module.
👨🏻🔧 Fixes
This fork contains fixes for those issues :
current_predicaterandom display #695 through Replace builtin map by ordered map for deterministic iteration prolog#2NewVariablein prolog interpreter #690 through Allow reset environnement global variable prolog#3👩💻 Related development
current_predicaterandom display #695, the implementation of an ordered map to register in a deterministic order the list of predicates in interpreter.MaxVariablesparameter in logic module limits params. By default,MaxVariableslimits has been set to100 000. The related commit message include the breaking changes marker (ab8f734).Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores