fix(x/warden): resolve space.owners for SignIntents
#159
Conversation
This comment has been minimized.
This comment has been minimized.
|
Warning Rate Limit Exceeded@Pitasi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 20 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 changes encompass enhancing context management in the 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 5
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (2)
- warden/x/warden/keeper/keys.go (2 hunks)
- warden/x/warden/keeper/shield.go (2 hunks)
Additional Context Used
Path-based Instructions (2)
warden/x/warden/keeper/keys.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.
| @@ -26,16 +27,16 @@ func NewKeysKeeper(sb *collections.SchemaBuilder, cdc codec.BinaryCodec) KeysKee | |||
| } | |||
| } | |||
|
|
|||
| func (k KeysKeeper) New(ctx sdk.Context, key types.Key, keyRequest types.KeyRequest) error { | |||
| func (k KeysKeeper) New(ctx context.Context, key types.Key, keyRequest types.KeyRequest) error { | |||
There was a problem hiding this comment.
Ensure proper error handling and logging for context cancellation in New method.
func (k KeysKeeper) New(ctx context.Context, key types.Key, keyRequest types.KeyRequest) error {
+ if ctx.Err() != nil {
+ return ctx.Err()
+ }
key.Id = keyRequest.Id
return k.Set(ctx, key)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (k KeysKeeper) New(ctx context.Context, key types.Key, keyRequest types.KeyRequest) error { | |
| func (k KeysKeeper) New(ctx context.Context, key types.Key, keyRequest types.KeyRequest) error { | |
| if ctx.Err() != nil { | |
| return ctx.Err() | |
| } | |
| key.Id = keyRequest.Id | |
| return k.Set(ctx, key) | |
| } |
| key.Id = keyRequest.Id | ||
| return k.Set(ctx, key) | ||
| } | ||
|
|
||
| func (k KeysKeeper) Get(ctx sdk.Context, id uint64) (types.Key, error) { | ||
| func (k KeysKeeper) Get(ctx context.Context, id uint64) (types.Key, error) { |
There was a problem hiding this comment.
Verify context cancellation handling in Get method.
func (k KeysKeeper) Get(ctx context.Context, id uint64) (types.Key, error) {
+ if ctx.Err() != nil {
+ return types.Key{}, ctx.Err()
+ }
return k.keys.Get(ctx, id)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (k KeysKeeper) Get(ctx context.Context, id uint64) (types.Key, error) { | |
| func (k KeysKeeper) Get(ctx context.Context, id uint64) (types.Key, error) { | |
| if ctx.Err() != nil { | |
| return types.Key{}, ctx.Err() | |
| } | |
| return k.keys.Get(ctx, id) | |
| } |
| return k.keys.Get(ctx, id) | ||
| } | ||
|
|
||
| func (k KeysKeeper) Set(ctx sdk.Context, key types.Key) error { | ||
| func (k KeysKeeper) Set(ctx context.Context, key types.Key) error { |
There was a problem hiding this comment.
Add context cancellation check in Set method to ensure graceful handling.
func (k KeysKeeper) Set(ctx context.Context, key types.Key) error {
+ if ctx.Err() != nil {
+ return ctx.Err()
+ }
if err := k.keysBySpace.Set(ctx, collections.Join(key.SpaceId, key.Id)); err != nil {
return err
}
return k.keys.Set(ctx, key.Id, key)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (k KeysKeeper) Set(ctx context.Context, key types.Key) error { | |
| func (k KeysKeeper) Set(ctx context.Context, key types.Key) error { | |
| if ctx.Err() != nil { | |
| return ctx.Err() | |
| } | |
| if err := k.keysBySpace.Set(ctx, collections.Join(key.SpaceId, key.Id)); err != nil { | |
| return err | |
| } | |
| return k.keys.Set(ctx, key.Id, key) | |
| } |
| @@ -20,7 +20,7 @@ func (w WardenShieldExpander) Expand(goCtx context.Context, ident *ast.Identifie | |||
| ctx := cosmoshield.UnwrapContext(goCtx) | |||
| msg := ctx.Msg() | |||
|
|
|||
| spaceID, err := extractSpaceID(msg) | |||
| spaceID, err := w.extractSpaceID(ctx, msg) | |||
There was a problem hiding this comment.
Consider adding detailed error messages to improve debugging in Expand method.
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to extract SpaceID: %w", err)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| spaceID, err := w.extractSpaceID(ctx, msg) | |
| spaceID, err := w.extractSpaceID(ctx, msg) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to extract SpaceID: %w", err) | |
| } |
warden/x/warden/keeper/shield.go
Outdated
| func (w WardenShieldExpander) extractSpaceID(ctx context.Context, msg sdk.Msg) (uint64, error) { | ||
| if msg, ok := msg.(getSpaceIder); ok { | ||
| return msg.GetSpaceId(), nil | ||
| } | ||
| if msg, ok := msg.(getKeyIder); ok { | ||
| keyID := msg.GetKeyId() | ||
| key, err := w.keeper.KeysKeeper.Get(ctx, keyID) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return key.SpaceId, nil | ||
| } |
There was a problem hiding this comment.
Refactor extractSpaceID to reduce nesting and improve readability.
func (w WardenShieldExpander) extractSpaceID(ctx context.Context, msg sdk.Msg) (uint64, error) {
- if msg, ok := msg.(getSpaceIder); ok {
- return msg.GetSpaceId(), nil
- }
- if msg, ok := msg.(getKeyIder); ok {
- keyID := msg.GetKeyId()
- key, err := w.keeper.KeysKeeper.Get(ctx, keyID)
- if err != nil {
- return 0, err
- }
- return key.SpaceId, nil
- }
- return 0, fmt.Errorf("message does not have a SpaceId field")
+ switch msg := msg.(type) {
+ case getSpaceIder:
+ return msg.GetSpaceId(), nil
+ case getKeyIder:
+ keyID := msg.GetKeyId()
+ key, err := w.keeper.KeysKeeper.Get(ctx, keyID)
+ if err != nil {
+ return 0, fmt.Errorf("failed to get key by ID: %w", err)
+ }
+ return key.SpaceId, nil
+ default:
+ return 0, fmt.Errorf("message does not have a SpaceId or KeyId field")
+ }
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (w WardenShieldExpander) extractSpaceID(ctx context.Context, msg sdk.Msg) (uint64, error) { | |
| if msg, ok := msg.(getSpaceIder); ok { | |
| return msg.GetSpaceId(), nil | |
| } | |
| if msg, ok := msg.(getKeyIder); ok { | |
| keyID := msg.GetKeyId() | |
| key, err := w.keeper.KeysKeeper.Get(ctx, keyID) | |
| if err != nil { | |
| return 0, err | |
| } | |
| return key.SpaceId, nil | |
| } | |
| func (w WardenShieldExpander) extractSpaceID(ctx context.Context, msg sdk.Msg) (uint64, error) { | |
| switch msg := msg.(type) { | |
| case getSpaceIder: | |
| return msg.GetSpaceId(), nil | |
| case getKeyIder: | |
| keyID := msg.GetKeyId() | |
| key, err := w.keeper.KeysKeeper.Get(ctx, keyID) | |
| if err != nil { | |
| return 0, fmt.Errorf("failed to get key by ID: %w", err) | |
| } | |
| return key.SpaceId, nil | |
| default: | |
| return 0, fmt.Errorf("message does not have a SpaceId or KeyId field") | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (1)
CHANGELOG.md (1)
52-58: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-98]
Review and correct grammatical errors and spelling mistakes throughout the CHANGELOG.md file. For instance, ensure proper capitalization and spelling of technical terms and identifiers.
Would you like assistance in performing a comprehensive grammar and spelling check on this document?
| @@ -52,6 +52,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| * (shield) [#148](https://github.com/warden-protocol/wardenprotocol/pull/148) Use protobufs for defining the AST | |||
| * (x/intent) [#155](https://github.com/warden-protocol/wardenprotocol/pull/155) Add MsgUpdateIntent, creators of an Intent can use it to change name and definition of their Intents. | |||
| * (x/warden) [#159](https://github.com/warden-protocol/wardenprotocol/pull/159) Resolve `warden.space.owners` in Intent definitions for MsgNewSignatureRequest and MsgNewSignTransactionRequest | |||
There was a problem hiding this comment.
Ensure the entry for PR #159 in the changelog is clear and accurately reflects the changes made.
- * (x/warden) [#159](https://github.com/warden-protocol/wardenprotocol/pull/159) Resolve `warden.space.owners` in Intent definitions for MsgNewSignatureRequest and MsgNewSignTransactionRequest
+ * (x/warden) [#159](https://github.com/warden-protocol/wardenprotocol/pull/159) Enhanced the resolution of `warden.space.owners` within Intent definitions to support `MsgNewSignatureRequest` and `MsgNewSignTransactionRequest`, improving module versatility.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * (x/warden) [#159](https://github.com/warden-protocol/wardenprotocol/pull/159) Resolve `warden.space.owners` in Intent definitions for MsgNewSignatureRequest and MsgNewSignTransactionRequest | |
| * (x/warden) [#159](https://github.com/warden-protocol/wardenprotocol/pull/159) Enhanced the resolution of `warden.space.owners` within Intent definitions to support `MsgNewSignatureRequest` and `MsgNewSignTransactionRequest`, improving module versatility. |
Intents referencing
warden.space.ownerswere only working when used for Actions that had aSpaceIdfield in the message, this is not the case forNewSignatureRequestandNewSignTransactionRequestthat only have aKeyIdfield instead.From the Key it's possible to go back to its Space, which is what PR adds to
x/wardenintent's expander.Summary by CodeRabbit