feat(x/intent)!: ability for modules to dynamically resolve variables on Action creation #139
Conversation
This allow us to pass other context to functions. In the future we might exploit this feature for enriching the sdk.Context with more fields.
Even if it was type-aliased (= re-exported), the depinject pkg from Cosmos SDK didn't like it. Since Environment is commonly accessed from outside of shield, it makes sense to move it outside the internal/ dir anyway.
In this version of shield we'll treat dots as part of the identifier (i.e. we won't dig much into building "javascript objects" equivalents). It'll be up to the Environment resolving the identifier to do whatever it prefers. E.g. `warden.space.owners` will be treated as a single name. The resolution env from x/warden can split by '.' and resolve 'space' properties individually.
This allows anyone to plug-in an implementer of the Expander interface to change the AST into another AST by replacing any Identifier with another AST node. This will be useful for resolving some Identifiers into Warden addresses and freeze them in time (e.g. when an Action is created).
…its string definition
We'll use the Expander later, when creating an Action, to process the Intent definition at the time the Action is created.
The ExpanderManager will dispatch Expander calls based on "namespace", i.e. the first part of an identifier (separated by a dot).
This way we can populate this field *after* preprocessing. The Intent stored in the database will have the original definition written by the user, the Intent stored inside the Action will be preprocessed (so the user knows exactly what's being evaluated to decide on its Action).
In this initial version, it's able to expand `space.owners` in the list of owners for the space. Only sdk.Msgs that have a SpaceId field are supported at this time.
|
Warning Rate Limit Exceeded@Pitasi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 53 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates bring significant changes to the system's intent and action handling, emphasizing dynamic variable resolution, AST node preprocessing, and identifier expansion. Notably, the shift towards dynamically resolved mentions enhances flexibility in intent definitions. The introduction of new structures like Changes
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 Configration File (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: .coderabbit.yml
Files ignored due to path filters (2)
warden/x/intent/types/action.pb.gois excluded by!**/*.pb.gowarden/x/intent/types/intent.pb.gois excluded by!**/*.pb.go
Files selected for processing (34)
- CHANGELOG.md (1 hunks)
- api/warden/intent/action.pulsar.go (17 hunks)
- api/warden/intent/intent.pulsar.go (14 hunks)
- proto/warden/intent/action.proto (1 hunks)
- proto/warden/intent/intent.proto (1 hunks)
- shield/ast/ast.go (4 hunks)
- shield/ast/expander.go (1 hunks)
- shield/ast/stringify.go (1 hunks)
- shield/env/env.go (1 hunks)
- shield/internal/evaluator/evaluator.go (4 hunks)
- shield/internal/lexer/lexer.go (2 hunks)
- shield/internal/metadata/metadata.go (1 hunks)
- shield/internal/metadata/metadata_test.go (1 hunks)
- shield/internal/parser/parser.go (1 hunks)
- shield/internal/parser/parser_test.go (1 hunks)
- shield/internal/preprocess/preprocess.go (1 hunks)
- shield/shield.go (1 hunks)
- warden/app/app.go (2 hunks)
- warden/repo/seqcollection.go (5 hunks)
- warden/testutil/keeper/intent.go (1 hunks)
- warden/x/intent/cosmoshield/context.go (1 hunks)
- warden/x/intent/cosmoshield/cosmosshield.go (1 hunks)
- warden/x/intent/keeper/actions.go (4 hunks)
- warden/x/intent/keeper/actions_keeper.go (1 hunks)
- warden/x/intent/keeper/intents.go (1 hunks)
- warden/x/intent/keeper/keeper.go (4 hunks)
- warden/x/intent/keeper/msg_server_new_intent.go (2 hunks)
- warden/x/intent/keeper/query_actions_by_address_test.go (1 hunks)
- warden/x/intent/migrations/v2/store.go (1 hunks)
- warden/x/intent/module/module.go (3 hunks)
- warden/x/intent/types/intent.go (1 hunks)
- warden/x/warden/keeper/shield.go (1 hunks)
- warden/x/warden/keeper/spaces.go (3 hunks)
- warden/x/warden/types/v1beta2/space.go (2 hunks)
Files skipped from review due to trivial changes (2)
- proto/warden/intent/intent.proto
- shield/internal/metadata/metadata_test.go
Additional Context Used
Path-based Instructions (31)
shield/env/env.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/migrations/v2/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/keeper/msg_server_new_intent.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/cosmoshield/context.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/ast/expander.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/types/intent.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/ast/stringify.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/preprocess/preprocess.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/metadata/metadata.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/keeper/query_actions_by_address_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"warden/testutil/keeper/intent.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/warden/keeper/shield.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/keeper/intents.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/repo/seqcollection.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/shield.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/cosmoshield/cosmosshield.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/keeper/actions_keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/warden/keeper/spaces.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/warden/types/v1beta2/space.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/parser/parser_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"warden/x/intent/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/lexer/lexer.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/evaluator/evaluator.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/ast/ast.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/intent/keeper/actions.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.shield/internal/parser/parser.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"warden/x/intent/module/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/app/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/warden/intent/intent.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/warden/intent/action.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (43)
shield/env/env.go (1)
5-6: LGTM! TheEnvironmentinterface is well-defined and documented.warden/x/intent/migrations/v2/store.go (1)
7-7: Ensure that the simplification of theMigrateStorefunction does not omit necessary migration steps for existing data.warden/x/intent/keeper/msg_server_new_intent.go (1)
11-18: Ensure that removing the logic for extracting addresses from theNewIntentfunction does not impact the system's ability to track entities involved in actions.warden/x/intent/cosmoshield/context.go (1)
9-34: Add documentation for theContextstruct and its methods to clarify their purpose and usage.shield/ast/expander.go (1)
5-17: LGTM! TheExpanderinterface is well-defined and documented, supporting dynamic variable resolution.warden/x/intent/types/intent.go (1)
11-15: Update the documentation for theEvalmethod to reflect the addition of thecontext.Contextparameter and its impact on handling dynamic contexts.shield/ast/stringify.go (1)
8-23: LGTM! TheStringifyfunction is well-implemented and covers various AST node types efficiently.shield/internal/metadata/metadata.go (1)
4-4: Ensure that the updated import pathgithub.com/warden-protocol/wardenprotocol/shield/astis correct and accessible.warden/x/intent/keeper/query_actions_by_address_test.go (1)
32-34: Replace theAddressesfield withMentionsin thetypes.Actionstruct to align with the new data model.This change reflects the PR's objective to enhance dynamic variable resolution by using
Mentionsinstead ofAddresses.warden/testutil/keeper/intent.go (1)
41-41: Adding anilargument to thekeeper.NewKeepercall is acceptable if the new parameter is optional and handled appropriately within theNewKeeperfunction.Ensure that the
NewKeeperfunction is designed to handlenilfor the newly added parameter without causing runtime errors.warden/x/intent/keeper/intents.go (2)
17-38: Ensure thatfreezeIntentproperly handles errors without causing panics.Consider returning errors to the caller instead of using
panicfor error handling. This approach allows for more graceful error handling and recovery.
41-51: Optimize theresolveAddressesfunction by directly returning the filtered list of valid addresses.This function efficiently filters and returns only valid Bech32 addresses from a list of identifiers, aligning with the PR's objectives.
warden/repo/seqcollection.go (1)
29-46: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [32-76]
Validate the use of
context.ContextinSeqCollectionmethods.Switching to
context.Contextfromsdk.Contextin method signatures is a good practice for broader compatibility and flexibility in handling request-scoped values, deadlines, and cancellation signals.shield/shield.go (2)
22-30: Ensure that theRunfunction correctly handles the context and expander parameters.The addition of
contextandexpanderparameters to theRunfunction enhances its flexibility and allows for dynamic AST node expansion, aligning with the PR's objectives.
52-58: Confirm thatExtractMetadatacorrectly extracts metadata from the AST.Refactoring
ValidatetoExtractMetadataand focusing on extracting metadata directly from the AST root enhances the system's ability to dynamically resolve variables and maintain action integrity.warden/x/intent/cosmoshield/cosmosshield.go (1)
12-72: Validate the implementation ofNamespaceExpanderandExpanderManager.The implementation of
NamespaceExpanderandExpanderManagerprovides a flexible mechanism for expanding identifiers based on their namespace, which is crucial for the dynamic resolution of variables likewarden.space.owners.warden/x/intent/keeper/actions_keeper.go (1)
58-58: Ensure that theMentionsfield ofActionis correctly populated with resolved addresses before this method is called.warden/x/warden/keeper/spaces.go (3)
31-31: Ensure all callers of theGetfunction are updated to passcontext.Contextinstead ofsdk.Context.Verification successful
The verification process confirms that all calls to the
Getfunction in theSpacesKeeperstruct are correctly updated to usecontext.Context. This aligns with the expectations set by the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to the `Get` function in the `SpacesKeeper` struct are updated to use `context.Context`. ast-grep --lang go --pattern $'$_.$_.Get($$$)'Length of output: 5535
35-35: Ensure all callers of theNewfunction are updated to passcontext.Contextinstead ofsdk.Context.
55-55: Ensure all callers of theupdateSpaceOwnersfunction are updated to passcontext.Contextinstead ofsdk.Context.warden/x/warden/types/v1beta2/space.go (1)
87-87: Ensure thatwarden.space.ownersis correctly resolved to actual owner addresses in the context whereAnyOwnerIntentis used.shield/internal/parser/parser_test.go (1)
7-7: Ensure that the new import path for theastpackage is consistently used throughout the codebase.Verification successful
The new import path for the
astpackage is consistently used throughout the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new import path for the `ast` package is used consistently across all files. rg --type go 'github.com/warden-protocol/wardenprotocol/shield/ast'Length of output: 1237
shield/ast/ast.go (3)
24-31: Ensure theNewIdentfunction correctly initializes anIdentifiernode with the provided name.
43-50: Ensure theNewIntfunction correctly initializes anIntegerLiteralnode with the provided integer value.
62-78: Ensure theNewBoolfunction correctly initializes aBooleanLiteralnode based on the provided boolean value.warden/x/intent/keeper/actions.go (3)
10-10: Ensure the new importgithub.com/warden-protocol/wardenprotocol/warden/x/intent/cosmoshieldis used correctly within the file.
47-47: Verify that thectxparameter is correctly utilized in theEvalmethod call withinCheckActionReady.
98-105: Ensure that thefreezeIntentfunction correctly processes theintentand updates itsDefinitionandmentionsfields based on the dynamic context.shield/internal/parser/parser.go (1)
7-7: Ensure the updated import path for theastpackage is correct and aligns with the project's structure.warden/x/intent/module/module.go (2)
24-25: Ensure the new imports are correctly utilized within the file.
206-225: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [190-215]
Verify that the
ShieldExpanderFuncfield is correctly optional and properly utilized to provide a defaultast.Expanderif not supplied.warden/app/app.go (2)
61-65: Ensure the new imports are correctly utilized within the file.
203-214: Verify that the closure returning anast.Expanderinstance is correctly implemented and integrates well with thex/intentmodule.api/warden/intent/intent.pulsar.go (1)
692-710: Ensure all references to the removedAddressesfield are also removed from any related code or documentation to maintain consistency and avoid confusion.api/warden/intent/action.pulsar.go (9)
569-613: Consider adding documentation comments for the_Action_12_listtype and its methods to improve code readability and maintainability.
624-630: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [627-643]
Ensure the
mentionsfield in theActionmessage descriptor is properly documented in the protobuf definition file to clarify its purpose and usage.
771-776: Verify that the handling of thementionslist in theActionstruct'sRangemethod aligns with the intended usage patterns and performance considerations, especially in contexts with a large number of mentions.
812-813: Ensure that theHasmethod's implementation for thementionsfield correctly reflects the intended semantics, particularly in distinguishing between an unpopulated list and a list with zero elements.
850-851: Review theClearmethod implementation for thementionsfield to ensure it aligns with the intended behavior, especially in terms of memory management and clearing references in a slice.
901-906: Examine theGetmethod for thementionsfield to ensure it returns the expected behavior, particularly in scenarios where the list might be modified after retrieval.
949-952: In theSetmethod for thementionsfield, ensure that the handling of the list respects the intended ownership semantics, especially regarding the sharing of slices between instances.
1004-1009: Review theMutablemethod for thementionsfield to confirm that it correctly supports the intended mutability semantics, particularly in scenarios requiring concurrent access or modification.
1057-1059: When creating a new element for thementionslist, ensure that theNewElementmethod's behavior aligns with the expected usage patterns, especially in terms of default values and initialization.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- warden/x/warden/keeper/shield.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- warden/x/warden/keeper/shield.go
Pitasi
changed the title
Pitasi
changed the title
In short: x/warden can now resolve
warden.space.ownersin Intent definitions into the list of owners of the space.To get there I added a new step to the Shield engine called "preprocessing" where identifiers can be handled by objects called "Expanders" and turned into new AST nodes.
During the Action creation, we now run the preprocessing step, and store the frozen Intent inside the Action.
Example: I have an intent saying
any(2, warden.space.owners). I create an Action, now I'm waiting for the second approvals from another owner. Even if I try to add a new Owner to the Space now, it wouldn't matter. The owners list is frozen at the time the Action was created.Another rationale for this behaviour is that we can easily send our notifications/events/etc. with the frozen list of "Mentions" for the Action.
Future work could involve storing the AST directly in a protobuf format, instead of parsing the string into AST, run preprocessing, and re-render the AST back into a string.
Summary by CodeRabbit
New Features
Refactor
Addressesfield from theIntentstruct, to align with updated functionality.Bug Fixes
context.Contextandsdk.Contextacross various functions to ensure consistent and correct context usage.Chores