Conversation
Pitasi
force-pushed
the
upgrade-evm-v0.6.0
branch
from
de518bd to
2e11612
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughBumps Go toolchain to 1.26 across CI/workflows and Dockerfiles, upgrades golangci-lint action/tooling, updates github.com/cosmos/evm to v0.6.0, migrates IBC transfer imports/keeper usage to ibc-go v10, and replaces many public API parameter types from Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 46: The changelog entry "x/evm: update cosmos/evm to v0.6.0." placed
under the "Consensus Breaking Changes" section must be rewritten to match the
documented format and include upgrade impact; change it to the required "(<tag>)
#<issue-number>" suffix (e.g., "(patch) `#1234`") and expand the description to
state the exact change (update of cosmos/evm to v0.6.0) plus a brief upgrade
impact line describing what users must do (e.g., state migration steps, state
compatibility or required node/client updates, and any downtime or
reconfiguration), keeping the entry concise and grammatically correct.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.github/workflows/go-client.yml.github/workflows/integration-tests.yml.github/workflows/keychain-sdk.yml.github/workflows/shield.yml.github/workflows/soliditygen.yml.github/workflows/wardend.yaml.github/workflows/wardenkms.ymlCHANGELOG.mdcmd/wardend/Dockerfilecmd/wardenkms/Dockerfilego.modwarden.Dockerfilewarden/app/app.gowarden/app/precompiles.go
| ### Consensus Breaking Changes | ||
|
|
||
| - sdk: update Cosmos SDK to v0.53.6. | ||
| - x/evm: update cosmos/evm to v0.6.0. |
Contributor
There was a problem hiding this comment.
Make the new changelog entry conform to the documented entry format and add impact context.
Line 46 is under Consensus Breaking Changes, but it currently omits the documented (<tag>) #<issue-number> format and lacks upgrade-impact detail.
📝 Proposed changelog update
-- x/evm: update cosmos/evm to v0.6.0.
+- (x/evm) `#1863` update cosmos/evm to v0.6.0 (consensus-breaking; see PR migration notes).As per coding guidelines, "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness".
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - x/evm: update cosmos/evm to v0.6.0. | |
| - (x/evm) `#1863` update cosmos/evm to v0.6.0 (consensus-breaking; see PR migration notes). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 46, The changelog entry "x/evm: update cosmos/evm to
v0.6.0." placed under the "Consensus Breaking Changes" section must be rewritten
to match the documented format and include upgrade impact; change it to the
required "(<tag>) #<issue-number>" suffix (e.g., "(patch) `#1234`") and expand the
description to state the exact change (update of cosmos/evm to v0.6.0) plus a
brief upgrade impact line describing what users must do (e.g., state migration
steps, state compatibility or required node/client updates, and any downtime or
reconfiguration), keeping the entry concise and grammatically correct.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
warden/x/act/client/actions.go (1)
270-277:⚠️ Potential issue | 🟠 MajorDo not exit field population early on empty
Anyflag.At Line 277,
return nilexitspopulateFromFlagsfor the whole message, so later fields are never parsed. This can silently drop user-provided flags.Proposed fix
case reflect.TypeFor[codectypes.Any](): jvalue, err := cmd.Flags().GetString(flagName) if err != nil { return err } if jvalue == "" { - return nil + continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warden/x/act/client/actions.go` around lines 270 - 277, The populateFromFlags function currently returns from the entire function when an Any-type flag value is empty inside the switch case reflect.TypeFor[codectypes.Any](), which prematurely stops parsing subsequent fields; change the behavior so an empty jvalue does not return but instead skips only this field (e.g., replace the early `return nil` with code that continues processing the rest of the fields or otherwise skips setting this field) in the case handling for codectypes.Any within populateFromFlags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@warden/x/act/client/actions.go`:
- Around line 270-277: The populateFromFlags function currently returns from the
entire function when an Any-type flag value is empty inside the switch case
reflect.TypeFor[codectypes.Any](), which prematurely stops parsing subsequent
fields; change the behavior so an empty jvalue does not return but instead skips
only this field (e.g., replace the early `return nil` with code that continues
processing the rest of the fields or otherwise skips setting this field) in the
case handling for codectypes.Any within populateFromFlags.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
cmd/clichain/main.goprecompiles/act/query.goprecompiles/act/tx.goprecompiles/async/query.goprecompiles/async/tx.goprecompiles/common/abi_encoding.goprecompiles/slinky/query.goprecompiles/warden/query.goprecompiles/warden/query_types.goprecompiles/warden/tx.goprecompiles/warden/tx_types.goprophet/plugins/http/http.goprophet/plugins/pricepred/pricepred_test.goshield/internal/evaluator/evaluator.gowarden/app/export.gowarden/x/act/client/actions.gowarden/x/act/keeper/query_actions_by_address_test.gowarden/x/act/types/v1beta1/expected_keepers.gowarden/x/async/keeper/query_plugin_metrics_by_id.gowarden/x/async/types/v1beta1/expected_keepers.gowarden/x/sched/types/v1beta1/expected_keepers.gowarden/x/warden/types/expected_keepers.gowarden/x/warden/types/v1beta3/space.go
✅ Files skipped from review due to trivial changes (3)
- prophet/plugins/http/http.go
- warden/app/export.go
- warden/x/async/keeper/query_plugin_metrics_by_id.go
Pitasi
force-pushed
the
upgrade-evm-v0.6.0
branch
from
d87d250 to
866992b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
warden/x/act/client/actions.go (1)
270-278:⚠️ Potential issue | 🟠 MajorDo not exit
populateFromFlagswhen an optionalAnyflag is empty.At Line 277,
return nilends the entire function, so remaining fields are not parsed if this field appears before them. This should skip only this field.Proposed fix
if jvalue == "" { - return nil + continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warden/x/act/client/actions.go` around lines 270 - 278, populateFromFlags currently returns from the whole function when an optional Any flag is empty (the code handling reflect.TypeFor[codectypes.Any]() uses "if jvalue == \"\" { return nil }"), which stops parsing remaining fields; change this behavior to skip only this field by replacing the early return with a skip/continue for the current switch/case (so the function proceeds to parse subsequent fields) — locate the case that matches reflect.TypeFor[codectypes.Any]() and the variables flagName and jvalue inside populateFromFlags and ensure an empty jvalue only skips that case rather than returning from populateFromFlags.precompiles/warden/query_types.go (1)
591-591:⚠️ Potential issue | 🟡 MinorFix incorrect unpacking error context string.
The message references
spacesInput, but this function unpacks intospacesByOwnerInput, which makes debugging misleading.Suggested fix
- return nil, fmt.Errorf("error while unpacking args to spacesInput struct: %w", err) + return nil, fmt.Errorf("error while unpacking args to spacesByOwnerInput struct: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@precompiles/warden/query_types.go` at line 591, The error message incorrectly references "spacesInput" when unpacking into the spacesByOwnerInput struct; update the fmt.Errorf call in the function that unpacks into spacesByOwnerInput to use "spacesByOwnerInput" (e.g., change the message in the return fmt.Errorf(...) to "error while unpacking args to spacesByOwnerInput struct: %w") so logs accurately reflect the target struct.
🧹 Nitpick comments (4)
precompiles/slinky/query.go (1)
81-83: Optional: standardize unpack errors with a typed precompile error.This path is correct, but using a shared typed error for unpack failures would align with the existing structured error approach in precompile handlers.
Based on learnings: In Go code within precompile methods (e.g., in
precompiles/act/query.go), prefer defining and using a custom error type (likeWrongArgsNumber) instead of usingfmt.Errorfdirectly for error messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@precompiles/slinky/query.go` around lines 81 - 83, Replace the generic fmt.Errorf used when method.Inputs.Copy fails with the project’s typed precompile error for argument unpacking (e.g., use the WrongArgsNumber or equivalent custom error type used in precompiles/act/query.go); locate the failing call in the get-price handler where method.Inputs.Copy(&input, args) returns an error and return the standardized precompile error (including the original error as context if the typed error supports wrapping) instead of fmt.Errorf to align with existing structured error handling.precompiles/act/tx.go (1)
64-67: Align type-mismatch errors with the existing typed-error pattern.Consider replacing these
fmt.Errorftype assertion errors with a shared custom error type, similar toWrongArgsNumber, so callers can reliably classify precompile input errors.Based on learnings: In Go code within precompile methods (e.g., in
precompiles/act/query.go), prefer defining and using a custom error type (likeWrongArgsNumber) instead of usingfmt.Errorfdirectly for error messages.Also applies to: 115-123, 172-175, 223-236, 291-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@precompiles/act/tx.go` around lines 64 - 67, Replace ad-hoc fmt.Errorf type-assertion errors with a shared typed error so callers can classify input errors; for the actionId assertion in tx.go (the block using actionId, ok := args[0].(uint64)) and the other assertion sites (around the other ranges mentioned), return a package-level custom error (e.g., WrongArgType or reuse the existing WrongArgsNumber pattern) instead of fmt.Errorf, and update the functions to use that error value/constructor so type-mismatch failures are consistently identifiable by callers.precompiles/act/query.go (1)
104-107: Use a shared typed error for invalid argument values/types.These paths currently return
fmt.Errorf, while arity already usesWrongArgsNumber. A dedicated typed error (e.g., invalid arg type/value) would make precompile error handling more consistent.Based on learnings: In Go code within precompile methods (e.g., in
precompiles/act/query.go), prefer defining and using a custom error type (likeWrongArgsNumber) instead of usingfmt.Errorfdirectly for error messages.Also applies to: 155-157, 254-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@precompiles/act/query.go` around lines 104 - 107, The code in precompiles/act/query.go returns plain fmt.Errorf for bad arg types (e.g., the actionId type assertion around args[0]) which is inconsistent with the existing WrongArgsNumber typed error; define a shared typed error value (for example InvalidArgType or ErrInvalidArg) and replace the fmt.Errorf calls at the actionId assertion site and the other spots mentioned (around the code blocks at the second and third assertion locations) to return that typed error (including the original context/message) so callers can identify invalid argument-type/value errors consistently; update any tests/consumers to expect the new typed error if needed.precompiles/warden/query_types.go (1)
334-337: Consider typed errors for argument type mismatches in precompile parsers.Using a dedicated error type (similar to
WrongArgsNumber) would make downstream handling/log filtering more consistent than ad-hocfmt.Errorfstrings.Based on learnings: In Go code within precompile methods (e.g., in
precompiles/act/query.go), prefer defining and using a custom error type (likeWrongArgsNumber) instead of usingfmt.Errorfdirectly for error messages. This enhances error handling and code clarity.Also applies to: 488-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@precompiles/warden/query_types.go` around lines 334 - 337, Replace the fmt.Errorf calls used for argument type mismatches with a typed error (either reuse the existing WrongArgsNumber pattern or add a new error type like WrongArgType) and return that typed error from the parser when args[0] cannot be asserted to uint64 (the id variable) and at the other occurrence around lines 488-491; specifically, locate the code that checks args and the id assertion (symbols: args and id in query_types.go) and change the error creation to return the typed error (including contextual info such as expected type and actual type) so downstream code can detect and handle this error via type assertions instead of parsing error strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/clichain/main.go`:
- Around line 68-75: The opened file handles that get assigned to the shared
reader variable in (the branch that opens keyFile and the other similar branch
that also opens a file and rebinds in) are never closed; after each successful
os.Open(keyFile) (where f is the *os.File), add a defer f.Close() immediately
after the open succeeds so the file is closed when the surrounding function
returns (do the same in the other branch that opens and assigns f to in).
---
Outside diff comments:
In `@precompiles/warden/query_types.go`:
- Line 591: The error message incorrectly references "spacesInput" when
unpacking into the spacesByOwnerInput struct; update the fmt.Errorf call in the
function that unpacks into spacesByOwnerInput to use "spacesByOwnerInput" (e.g.,
change the message in the return fmt.Errorf(...) to "error while unpacking args
to spacesByOwnerInput struct: %w") so logs accurately reflect the target struct.
In `@warden/x/act/client/actions.go`:
- Around line 270-278: populateFromFlags currently returns from the whole
function when an optional Any flag is empty (the code handling
reflect.TypeFor[codectypes.Any]() uses "if jvalue == \"\" { return nil }"),
which stops parsing remaining fields; change this behavior to skip only this
field by replacing the early return with a skip/continue for the current
switch/case (so the function proceeds to parse subsequent fields) — locate the
case that matches reflect.TypeFor[codectypes.Any]() and the variables flagName
and jvalue inside populateFromFlags and ensure an empty jvalue only skips that
case rather than returning from populateFromFlags.
---
Nitpick comments:
In `@precompiles/act/query.go`:
- Around line 104-107: The code in precompiles/act/query.go returns plain
fmt.Errorf for bad arg types (e.g., the actionId type assertion around args[0])
which is inconsistent with the existing WrongArgsNumber typed error; define a
shared typed error value (for example InvalidArgType or ErrInvalidArg) and
replace the fmt.Errorf calls at the actionId assertion site and the other spots
mentioned (around the code blocks at the second and third assertion locations)
to return that typed error (including the original context/message) so callers
can identify invalid argument-type/value errors consistently; update any
tests/consumers to expect the new typed error if needed.
In `@precompiles/act/tx.go`:
- Around line 64-67: Replace ad-hoc fmt.Errorf type-assertion errors with a
shared typed error so callers can classify input errors; for the actionId
assertion in tx.go (the block using actionId, ok := args[0].(uint64)) and the
other assertion sites (around the other ranges mentioned), return a
package-level custom error (e.g., WrongArgType or reuse the existing
WrongArgsNumber pattern) instead of fmt.Errorf, and update the functions to use
that error value/constructor so type-mismatch failures are consistently
identifiable by callers.
In `@precompiles/slinky/query.go`:
- Around line 81-83: Replace the generic fmt.Errorf used when method.Inputs.Copy
fails with the project’s typed precompile error for argument unpacking (e.g.,
use the WrongArgsNumber or equivalent custom error type used in
precompiles/act/query.go); locate the failing call in the get-price handler
where method.Inputs.Copy(&input, args) returns an error and return the
standardized precompile error (including the original error as context if the
typed error supports wrapping) instead of fmt.Errorf to align with existing
structured error handling.
In `@precompiles/warden/query_types.go`:
- Around line 334-337: Replace the fmt.Errorf calls used for argument type
mismatches with a typed error (either reuse the existing WrongArgsNumber pattern
or add a new error type like WrongArgType) and return that typed error from the
parser when args[0] cannot be asserted to uint64 (the id variable) and at the
other occurrence around lines 488-491; specifically, locate the code that checks
args and the id assertion (symbols: args and id in query_types.go) and change
the error creation to return the typed error (including contextual info such as
expected type and actual type) so downstream code can detect and handle this
error via type assertions instead of parsing error strings.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
cmd/clichain/main.goprecompiles/act/query.goprecompiles/act/tx.goprecompiles/async/query.goprecompiles/async/tx.goprecompiles/common/abi_encoding.goprecompiles/slinky/query.goprecompiles/warden/query.goprecompiles/warden/query_types.goprecompiles/warden/tx.goprecompiles/warden/tx_types.goprophet/plugins/http/http.goprophet/plugins/pricepred/pricepred_test.goshield/internal/evaluator/evaluator.gowarden/app/export.gowarden/app/wasm-interop/custom_msg.gowarden/app/wasm-interop/custom_query.gowarden/x/act/client/actions.gowarden/x/act/keeper/query_actions_by_address_test.gowarden/x/act/types/v1beta1/expected_keepers.gowarden/x/async/keeper/query_plugin_metrics_by_id.gowarden/x/async/types/v1beta1/expected_keepers.gowarden/x/sched/types/v1beta1/expected_keepers.gowarden/x/warden/types/expected_keepers.gowarden/x/warden/types/v1beta3/space.go
🚧 Files skipped from review as they are similar to previous changes (6)
- shield/internal/evaluator/evaluator.go
- precompiles/async/query.go
- warden/x/warden/types/expected_keepers.go
- warden/x/async/keeper/query_plugin_metrics_by_id.go
- prophet/plugins/pricepred/pricepred_test.go
- warden/x/sched/types/v1beta1/expected_keepers.go
Comment on lines
68
to
75
| if keyFile != "" { | ||
| f, err := os.Open(keyFile) | ||
| if err != nil { | ||
| return fmt.Errorf("opening key file: %w", err) | ||
| } | ||
|
|
||
| in = f | ||
| } |
Contributor
There was a problem hiding this comment.
Close optional input files after opening.
Both branches open a file and rebind in, but never close the file handle. Add defer f.Close() in each branch.
Suggested fix
@@
if keyFile != "" {
f, err := os.Open(keyFile)
if err != nil {
return fmt.Errorf("opening key file: %w", err)
}
+ defer f.Close()
in = f
}
@@
if messageFile != "" {
f, err := os.Open(messageFile)
if err != nil {
return fmt.Errorf("opening message file: %w", err)
}
+ defer f.Close()
in = f
}Also applies to: 142-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/clichain/main.go` around lines 68 - 75, The opened file handles that get
assigned to the shared reader variable in (the branch that opens keyFile and the
other similar branch that also opens a file and rebinds in) are never closed;
after each successful os.Open(keyFile) (where f is the *os.File), add a defer
f.Close() immediately after the open succeeds so the file is closed when the
surrounding function returns (do the same in the other branch that opens and
assigns f to in).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR upgrade cosmos/evm to its latest version (v0.6.0).