Conversation
WalkthroughThis PR introduces a lease reclamation feature for the market module, adds node version discovery through an Akash client registry, establishes upgrade migrations for v2.1.0 (including default reclamation parameters), and defines agent skill guidelines. Changes span deployment and market handlers, keeper method signatures, extensive test coverage, and infrastructure setup. Changes
Sequence DiagramsequenceDiagram
participant Tenant
participant Deployment as Deployment<br/>Handler
participant Market as Market<br/>Keeper
participant Lease as Lease<br/>State
participant Reclaim as Reclamation<br/>Window
Tenant->>Deployment: CreateDeployment(reclamation)
Deployment->>Market: GetParams()
Deployment->>Deployment: Validate reclamation<br/>against min/max
Deployment->>Market: CreateOrder(reclamation)
Market->>Lease: Store order.Reclamation
Tenant->>Deployment: StartGroup()
Deployment->>Lease: Load deployment
Deployment->>Market: CreateOrder(deployment.Reclamation)
Market->>Lease: Store bid.ReclamationWindow
Tenant->>Market: LeaseStartReclaim(leaseID)
Market->>Lease: Transition LeaseActive→LeaseReclaiming
Lease->>Reclaim: Set StartedAt, Deadline,<br/>Reason
Market->>Lease: Emit EventLeaseReclaimStarted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
.agents/skills/upgrade-changelog/SKILL.md (1)
79-80: Prefer repo-local constant resolution before module cache/go doc.Relying on module cache or
go doccan make runs environment-dependent. Recommend explicitly prioritizing repository-local lookup first, then fallback to cache/go doc.Suggested wording update
-- When resolving Go constants from external packages, search under the Go module cache or use `go doc` if needed. The packages typically follow the pattern `pkg.akt.dev/go/node/<module>/<version>`. +- When resolving Go constants, first search repository-local sources (including vendored/copied module code if present). If not found, search the Go module cache or use `go doc` as fallback. The packages typically follow the pattern `pkg.akt.dev/go/node/<module>/<version>`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/upgrade-changelog/SKILL.md around lines 79 - 80, Update the constant-resolution logic to prefer repository-local lookup before falling back to the Go module cache or invoking `go doc`: first search the repo source tree for the package and constant (e.g., resolve `epochstypes.StoreKey` within local packages), then if not found search the module cache pattern `pkg.akt.dev/go/node/<module>/<version>` or call `go doc`; if still unresolved, keep using the raw Go expression placeholder (e.g., `` `epochstypes.StoreKey` ``) and emit a user warning.x/oracle/keeper/abci.go (1)
163-165: Align error log field naming with the rest of the file.At Line 164, prefer the same structured field convention used elsewhere (
"error", err) for consistent log querying.Suggested refactor
err = k.aggregatedPrices.Set(sctx, did, aggregatedPrice) if err != nil { - sctx.Logger().Error("set aggregated price", "reason", err.Error()) + sctx.Logger().Error("set aggregated price", "error", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/oracle/keeper/abci.go` around lines 163 - 165, Replace the structured log field "reason" with the consistent key "error" in the sctx.Logger().Error call and pass the error value (err) itself rather than err.Error(); locate the call to sctx.Logger().Error that logs "set aggregated price" and change the field tuple from ("reason", err.Error()) to ("error", err) to match the rest of the file's logging convention.tests/upgrade/docker-compose.hermes.yaml (2)
46-47: Remove redundant port mapping with host networking.Port mappings are ignored when using
network_mode: "host"because the container shares the host's network namespace directly. The service will automatically bind to port 3000 on the host without explicit mapping.♻️ Proposed cleanup
env_file: - ${AKASH_RUN_DIR}/hermes.env - ports: - - "3000:3000" entrypoint: ["/bin/sh", "-c"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/upgrade/docker-compose.hermes.yaml` around lines 46 - 47, The docker-compose service currently declares a host network mode and also a redundant ports mapping; remove the "ports:" entry (specifically the "- \"3000:3000\"" mapping) from the service configuration so it relies on network_mode: "host" instead, ensuring no leftover ports: block remains; look for the service block containing network_mode: "host" and the ports: key to update.
29-29: Consider pinning the Hermes image version.Using the
latesttag can lead to unpredictable behavior and non-reproducible builds. Consider pinning to a specific version or commit SHA for consistency across test environments.📌 Example with version pinning
- image: ghcr.io/akash-network/hermes:latest + image: ghcr.io/akash-network/hermes:v1.2.3 # Replace with actual version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/upgrade/docker-compose.hermes.yaml` at line 29, The docker-compose entry uses an unstable tag "image: ghcr.io/akash-network/hermes:latest"; change that to a pinned version or digest (for example replace with "image: ghcr.io/akash-network/hermes:<semver-or-sha>" or "image: ghcr.io/akash-network/hermes@sha256:<digest>") to ensure reproducible test runs, or make the tag configurable via an environment variable (e.g., use "${HERMES_IMAGE}") and document/update the .env/CI variables accordingly.go.mod (1)
62-62: Align Sonicrequireandreplaceversions to eliminate dependency-report drift.Line 62 replaces all
github.com/bytedance/sonicversions withv1.15.0, but line 121 still requiresv1.14.2indirect. While the replace directive correctly overrides at resolution time, this mismatch creates confusion in SBOM and security scanning outputs.Update line 121 to
v1.15.0and rungo mod tidyto keepgo.mod/go.sumconsistent:Proposed cleanup
- github.com/bytedance/sonic v1.14.2 // indirect + github.com/bytedance/sonic v1.15.0 // indirectThen refresh module metadata (
go mod tidy) sogo.mod/go.sumreflect actual resolved versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 62, Update the mismatched Sonic version in go.mod by changing the require entry for github.com/bytedance/sonic (currently v1.14.2) to v1.15.0 to match the replace directive, then run `go mod tidy` to refresh module metadata and regenerate go.sum so SBOM/security scans reflect the resolved v1.15.0 version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/guidelines/SKILL.md:
- Around line 59-63: The fenced code block that currently starts with ``` (the
three-backtick block showing the numbered verification steps) lacks a language
tag and triggers markdownlint MD040; fix it by adding a language identifier
(e.g., "text") immediately after the opening backticks so the block becomes
```text, ensuring the code fence is tagged without changing the block contents.
In @.agents/skills/upgrade-changelog/SKILL.md:
- Around line 43-45: The fenced code block containing the text "Add new upgrades
after this line based on the template above" is missing a language tag which
triggers markdownlint MD040; update the opening fence in SKILL.md for that block
(the triple-backtick block shown in the diff) to include a language tag such as
```text so the block becomes ```text and keep the closing ``` intact.
In `@make/test-upgrade.mk`:
- Line 63: The Makefile `test` target currently comments out its `init`
prerequisite (`test: `#init``) so `make test` runs without initializing required
state; change the target declaration to include the `init` prerequisite (e.g.,
`test: init`) so `init` runs before tests and ensures dependencies like
`$(COSMOVISOR)`, `$(AKASH_INIT)` and the `$(AP_RUN_DIR)/validators` state are
created; update any related comments if necessary to reflect that `init` is a
real prerequisite and not commented out.
In `@tests/upgrade/docker-compose.hermes.yaml`:
- Around line 38-39: The comment above UPDATE_INTERVAL_MS is inconsistent with
its value; update either the comment or the value: either change the comment to
state "10 seconds for testing" to match UPDATE_INTERVAL_MS=10000, or set
UPDATE_INTERVAL_MS=60000 to match "1 minute for testing"; locate the
UPDATE_INTERVAL_MS entry in the docker-compose.hermes.yaml and make the comment
and the numeric value consistent.
- Around line 32-35: The container is using network_mode: "host" while
RPC_ENDPOINT is set to host.docker.internal, which is incompatible; update the
configuration so the RPC endpoint matches host networking by replacing the
RPC_ENDPOINT value (RPC_ENDPOINT=http://host.docker.internal:26657) with the
loopback host used with host networking (e.g.,
RPC_ENDPOINT=http://127.0.0.1:26657), or alternatively remove/disable
network_mode: "host" and keep RPC_ENDPOINT as host.docker.internal to run the
container on the bridge network.
In `@testutil/network/rpc.go`:
- Around line 24-28: Tests call aclient.DiscoverClient which relies on
aclient.GetRegistry(), but the registry isn't initialized in the test app path;
update the test app construction to call aclient.SetRegistry(...) after creating
the app so GetRegistry() returns a populated registry. Specifically, add
aclient.SetRegistry(...) in the test setup flow (for example in
testutil/types.go immediately after app.NewApp() returns, or in startInProcess()
before LocalRPCClient.Akash() / DiscoverClient() are used) so
LocalRPCClient.Akash(), aclient.GetRegistry(), and DiscoverClient() see a proper
registry.
In `@x/market/handler/handler_reclamation_test.go`:
- Around line 110-126: The helper createLeaseWithReclamation currently writes
lease.Reclamation directly into the store after calling
MarketKeeper().CreateLease, which prevents tests from verifying that
msgServer.CreateLease actually copies the bid's ReclamationWindow onto the
lease; to fix, remove the direct store mutation in createLeaseWithReclamation so
it only creates the bid (via createBidWithReclamation) and calls
MarketKeeper().CreateLease, then return the bid/order/lease id for callers to
assert propagation; if tests need a helper that force-injects reclamation for
other scenarios, create a separate helper (e.g., injectReclamationOnLease) that
uses GetLease and SaveLease to set lease.Reclamation, leaving
TestCreateLease_StoresReclamation to call CreateLease and then read the lease
from MarketKeeper().GetLease to assert reclamation was set.
In `@x/market/module.go`:
- Around line 182-184: ConsensusVersion for the market module was bumped to 9
(AppModule.ConsensusVersion()) but no migration handler was registered, so
register the market module migration for v8→v9 by calling
utypes.RegisterMigration(...) during the upgrade initialization (same place
other modules register theirs in the upgrade init flow) and point it to the
migration handler implemented in upgrades software v2.1.0 (the function that
sets reclamation params); ensure the registered migration key/version matches
the market module and new consensus version so the handler is invoked during the
node upgrade.
In `@x/oracle/keeper/abci.go`:
- Around line 155-157: The error log inside the aggregation failure branch
currently drops the error object; update the error handling in the block where
sctx.Logger().Error("calculate aggregated price") is called (after the
aggregated price calculation that sets err) to include the actual err value and
relevant context (e.g., denom or request identifiers) in the log entry so the
logger records the error details for debugging.
---
Nitpick comments:
In @.agents/skills/upgrade-changelog/SKILL.md:
- Around line 79-80: Update the constant-resolution logic to prefer
repository-local lookup before falling back to the Go module cache or invoking
`go doc`: first search the repo source tree for the package and constant (e.g.,
resolve `epochstypes.StoreKey` within local packages), then if not found search
the module cache pattern `pkg.akt.dev/go/node/<module>/<version>` or call `go
doc`; if still unresolved, keep using the raw Go expression placeholder (e.g.,
`` `epochstypes.StoreKey` ``) and emit a user warning.
In `@go.mod`:
- Line 62: Update the mismatched Sonic version in go.mod by changing the require
entry for github.com/bytedance/sonic (currently v1.14.2) to v1.15.0 to match the
replace directive, then run `go mod tidy` to refresh module metadata and
regenerate go.sum so SBOM/security scans reflect the resolved v1.15.0 version.
In `@tests/upgrade/docker-compose.hermes.yaml`:
- Around line 46-47: The docker-compose service currently declares a host
network mode and also a redundant ports mapping; remove the "ports:" entry
(specifically the "- \"3000:3000\"" mapping) from the service configuration so
it relies on network_mode: "host" instead, ensuring no leftover ports: block
remains; look for the service block containing network_mode: "host" and the
ports: key to update.
- Line 29: The docker-compose entry uses an unstable tag "image:
ghcr.io/akash-network/hermes:latest"; change that to a pinned version or digest
(for example replace with "image: ghcr.io/akash-network/hermes:<semver-or-sha>"
or "image: ghcr.io/akash-network/hermes@sha256:<digest>") to ensure reproducible
test runs, or make the tag configurable via an environment variable (e.g., use
"${HERMES_IMAGE}") and document/update the .env/CI variables accordingly.
In `@x/oracle/keeper/abci.go`:
- Around line 163-165: Replace the structured log field "reason" with the
consistent key "error" in the sctx.Logger().Error call and pass the error value
(err) itself rather than err.Error(); locate the call to sctx.Logger().Error
that logs "set aggregated price" and change the field tuple from ("reason",
err.Error()) to ("error", err) to match the rest of the file's logging
convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68dfe338-e947-4264-88fc-ebcc07eeb316
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.agents/skills/guidelines/SKILL.md.agents/skills/upgrade-changelog/SKILL.mdapp/app.gocmd/akash/cmd/app_creator.gogo.modmake/test-upgrade.mktests/upgrade/docker-compose.hermes.yamltestutil/network/rpc.goupgrades/software/v2.1.0/upgrade.gox/deployment/handler/server.gox/deployment/imports/keepers.gox/market/handler/handler.gox/market/handler/handler_reclamation_test.gox/market/handler/handler_test.gox/market/handler/server.gox/market/keeper/grpc_query.gox/market/keeper/keeper.gox/market/keeper/keeper_reclamation_test.gox/market/keeper/keeper_test.gox/market/module.gox/market/simulation/genesis.gox/oracle/keeper/abci.go
Comment on lines
+59
to
+63
| ``` | ||
| 1. [Step] → verify: [check] | ||
| 2. [Step] → verify: [check] | ||
| 3. [Step] → verify: [check] | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block (markdownlint MD040).
Line 59 opens a fenced block without a language identifier, which will fail common markdown linting rules.
Suggested fix
-```
+```text
1. [Step] → verify: [check]
2. [Step] → verify: [check]
3. [Step] → verify: [check]</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/guidelines/SKILL.md around lines 59 - 63, The fenced code
block that currently starts with (the three-backtick block showing the numbered verification steps) lacks a language tag and triggers markdownlint MD040; fix it by adding a language identifier (e.g., "text") immediately after the opening backticks so the block becomestext, ensuring the code fence is
tagged without changing the block contents.
</details>
<!-- fingerprinting:phantom:triton:hawk:753d3d17-a651-49d5-85d3-9cb9da9cac21 -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+43
to
+45
| ``` | ||
| Add new upgrades after this line based on the template above | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced block at Line 43.
This block is missing a fence language and triggers markdownlint MD040.
Proposed fix
- ```
+ ```text
Add new upgrades after this line based on the template above
```📝 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
| ``` | |
| Add new upgrades after this line based on the template above | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/upgrade-changelog/SKILL.md around lines 43 - 45, The fenced
code block containing the text "Add new upgrades after this line based on the
template above" is missing a language tag which triggers markdownlint MD040;
update the opening fence in SKILL.md for that block (the triple-backtick block
shown in the diff) to include a language tag such as ```text so the block
becomes ```text and keep the closing ``` intact.
|
|
||
| .PHONY: test | ||
| test: init | ||
| test: #init |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the relevant section of make/test-upgrade.mk
cat -n make/test-upgrade.mk | head -80Repository: akash-network/node
Length of output: 3505
🏁 Script executed:
# Check if _run/common.mk exists and examine the init target
find . -name "common.mk" -type f | head -5Repository: akash-network/node
Length of output: 79
🏁 Script executed:
# Look for all references to the init target in Makefiles
rg "^init:" --type makefileRepository: akash-network/node
Length of output: 95
🏁 Script executed:
# Read _run/common.mk to see the init target definition
sed -n '30,50p' _run/common.mkRepository: akash-network/node
Length of output: 597
🏁 Script executed:
# Search for the init target definition in all Makefiles
rg "^init:" -A 1Repository: akash-network/node
Length of output: 195
🏁 Script executed:
# Check if there are any comments or git history related to this change
git log --oneline -n 20 -- make/test-upgrade.mk 2>/dev/null || echo "Git history not available"Repository: akash-network/node
Length of output: 107
🏁 Script executed:
# Check what COSMOVISOR resolves to
grep "COSMOVISOR\s*:=" make/test-upgrade.mk || grep "COSMOVISOR\s*:=" _run/common.mk || echo "Not found in test-upgrade.mk"Repository: akash-network/node
Length of output: 91
test target missing init prerequisite will cause test failures.
Line 63 has test: #init`` which comments out the init prerequisite. The `test` target then runs without the dependency chain that initializes the upgrade environment:
initdepends on$(COSMOVISOR)(binary) and$(AKASH_INIT)(initialization state)- The test command uses
$(AP_RUN_DIR)/validatorsand other state created only by runninginit
Without this prerequisite, make test fails unless make init has been manually run beforehand.
Suggested fix
.PHONY: test
-test: `#init`
+test: init📝 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
| test: #init | |
| .PHONY: test | |
| test: init |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@make/test-upgrade.mk` at line 63, The Makefile `test` target currently
comments out its `init` prerequisite (`test: `#init``) so `make test` runs without
initializing required state; change the target declaration to include the `init`
prerequisite (e.g., `test: init`) so `init` runs before tests and ensures
dependencies like `$(COSMOVISOR)`, `$(AKASH_INIT)` and the
`$(AP_RUN_DIR)/validators` state are created; update any related comments if
necessary to reflect that `init` is a real prerequisite and not commented out.
Comment on lines
+38
to
+39
| # Update interval (1 minute for testing, use 5+ minutes in production) | ||
| - UPDATE_INTERVAL_MS=10000 |
There was a problem hiding this comment.
Align comment with actual interval value.
The comment states "1 minute for testing" but the actual value is 10000 ms (10 seconds). Either update the comment to reflect the 10-second interval or change the value to 60000 ms for 1 minute.
📝 Proposed fix to match comment
- # Update interval (1 minute for testing, use 5+ minutes in production)
- - UPDATE_INTERVAL_MS=10000
+ # Update interval (10 seconds for testing, use 5+ minutes in production)
+ - UPDATE_INTERVAL_MS=10000Or to match the comment:
# Update interval (1 minute for testing, use 5+ minutes in production)
- - UPDATE_INTERVAL_MS=10000
+ - UPDATE_INTERVAL_MS=60000📝 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
| # Update interval (1 minute for testing, use 5+ minutes in production) | |
| - UPDATE_INTERVAL_MS=10000 | |
| # Update interval (10 seconds for testing, use 5+ minutes in production) | |
| - UPDATE_INTERVAL_MS=10000 |
Suggested change
| # Update interval (1 minute for testing, use 5+ minutes in production) | |
| - UPDATE_INTERVAL_MS=10000 | |
| # Update interval (1 minute for testing, use 5+ minutes in production) | |
| - UPDATE_INTERVAL_MS=60000 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/upgrade/docker-compose.hermes.yaml` around lines 38 - 39, The comment
above UPDATE_INTERVAL_MS is inconsistent with its value; update either the
comment or the value: either change the comment to state "10 seconds for
testing" to match UPDATE_INTERVAL_MS=10000, or set UPDATE_INTERVAL_MS=60000 to
match "1 minute for testing"; locate the UPDATE_INTERVAL_MS entry in the
docker-compose.hermes.yaml and make the comment and the numeric value
consistent.
Comment on lines
+110
to
+126
| func (st *testSuite) createLeaseWithReclamation(reclaimWindow *time.Duration) (mv1.LeaseID, mvbeta.Bid, mvbeta.Order) { | ||
| st.t.Helper() | ||
| bid, order := st.createBidWithReclamation(reclaimWindow) | ||
|
|
||
| err := st.MarketKeeper().CreateLease(st.Context(), bid) | ||
| require.NoError(st.t, err) | ||
|
|
||
| // Store reclamation on the lease if bid has it | ||
| if reclaimWindow != nil { | ||
| lease, found := st.MarketKeeper().GetLease(st.Context(), bid.ID.LeaseID()) | ||
| require.True(st.t, found) | ||
| lease.Reclamation = &mv1.Reclamation{ | ||
| Window: *reclaimWindow, | ||
| } | ||
| err = st.MarketKeeper().SaveLease(st.Context(), lease) | ||
| require.NoError(st.t, err) | ||
| } |
There was a problem hiding this comment.
Split this helper from propagation assertions.
This helper writes lease.Reclamation directly into the store after MarketKeeper().CreateLease, so any test that uses it cannot verify that msgServer.CreateLease actually copied bid.ReclamationWindow onto the lease. That makes TestCreateLease_StoresReclamation a false positive: it will still pass if the new handler logic regresses, and CloseBid would then stop enforcing reclamation because it only gates on lease.Reclamation != nil.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/market/handler/handler_reclamation_test.go` around lines 110 - 126, The
helper createLeaseWithReclamation currently writes lease.Reclamation directly
into the store after calling MarketKeeper().CreateLease, which prevents tests
from verifying that msgServer.CreateLease actually copies the bid's
ReclamationWindow onto the lease; to fix, remove the direct store mutation in
createLeaseWithReclamation so it only creates the bid (via
createBidWithReclamation) and calls MarketKeeper().CreateLease, then return the
bid/order/lease id for callers to assert propagation; if tests need a helper
that force-injects reclamation for other scenarios, create a separate helper
(e.g., injectReclamationOnLease) that uses GetLease and SaveLease to set
lease.Reclamation, leaving TestCreateLease_StoresReclamation to call CreateLease
and then read the lease from MarketKeeper().GetLease to assert reclamation was
set.
Comment on lines
182
to
184
| func (am AppModule) ConsensusVersion() uint64 { | ||
| return 8 | ||
| return 9 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for migration registrations in the market module
# Expect to find a RegisterMigration call for version 8 or migrations from 8 to 9
echo "=== Searching for migration registrations in market module ==="
rg -n -C5 'RegisterMigration' x/market/
echo ""
echo "=== Searching for migration files in market module ==="
fd -e go migration x/market/
echo ""
echo "=== Searching for version 8 or 9 references in market migrations ==="
rg -n 'version.*[89]|[89].*version|fromVersion|toVersion' x/market/Repository: akash-network/node
Length of output: 251
🏁 Script executed:
#!/bin/bash
# First, let's look at the full x/market/module.go to understand the module structure
echo "=== Content of x/market/module.go ==="
wc -l x/market/module.go
echo ""
# Let's read the module.go file to see how migrations are handled
head -200 x/market/module.go | tail -100
echo ""
echo "=== Search for migration handler registration patterns ==="
# Look for any migration-related setup in market module
rg -n 'migration|Migration' x/market/module.go -A 3 -B 3
echo ""
echo "=== Search for market migrations in broader codebase ==="
# Search for market-related migrations outside x/market/
find . -type f -name "*.go" -path "*/migrations/*" -exec rg -l "market|Market" {} \;
echo ""
echo "=== Search for v8 or v9 consensus version handling ==="
rg -n 'ConsensusVersion.*9|consensus.*version.*9' x/market/ -B 5 -A 5Repository: akash-network/node
Length of output: 3426
🏁 Script executed:
#!/bin/bash
# Let's understand how migrations are registered in this codebase
echo "=== Search for migration-related patterns in codebase ==="
rg -n 'RegisterMigration|migrations.NewMigrationHandler|MigrationHandler' --max-count 20
echo ""
echo "=== Look for any migrations directory ==="
fd -t d migrations
echo ""
echo "=== Check if market module has a migrations subdirectory ==="
ls -la x/market/ 2>/dev/null | grep -i migr || echo "No migrations directory in x/market"
echo ""
echo "=== Search for how other modules handle consensus version changes ==="
rg -n 'ConsensusVersion.*return [0-9]' x/ -B 2 -A 2 | head -40
echo ""
echo "=== Check if there's a migrations package in x/market ==="
find x/market -type f -name "*.go" | xargs grep -l "migration" 2>/dev/null || echo "No migration-related go files in x/market"
echo ""
echo "=== Search for v8 -> v9 migration in entire codebase ==="
rg -n 'v8|version.*8.*9|8.*9' x/market/Repository: akash-network/node
Length of output: 1677
🏁 Script executed:
#!/bin/bash
# Let's check what upgrade versions exist
echo "=== List all upgrade versions in upgrades/software/ ==="
ls -d upgrades/software/v*/
echo ""
echo "=== Search all upgrade version files for market migrations ==="
grep -r "market\|Market" upgrades/software/ --include="*.go" -n
echo ""
echo "=== Check the init.go files in each upgrade version ==="
for version_dir in upgrades/software/v*/; do
echo "=== $version_dir ==="
if [ -f "$version_dir/init.go" ]; then
cat "$version_dir/init.go"
fi
echo ""
doneRepository: akash-network/node
Length of output: 1300
Register migration handler for market module consensus version 8 → 9.
The ConsensusVersion increment to 9 lacks a corresponding migration handler registration. While market migration logic exists in upgrades/software/v2.1.0/upgrade.go (setting reclamation params), it must be explicitly registered via utypes.RegisterMigration() in the upgrade's init.go file, similar to the oracle and deployment migrations. Without this registration, the migration handler will not be invoked during node upgrades, risking state inconsistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x/market/module.go` around lines 182 - 184, ConsensusVersion for the market
module was bumped to 9 (AppModule.ConsensusVersion()) but no migration handler
was registered, so register the market module migration for v8→v9 by calling
utypes.RegisterMigration(...) during the upgrade initialization (same place
other modules register theirs in the upgrade init flow) and point it to the
migration handler implemented in upgrades software v2.1.0 (the function that
sets reclamation params); ensure the registered migration key/version matches
the market module and new consensus version so the handler is invoked during the
node upgrade.
Signed-off-by: Artur Troian <troian@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.agents/skills/upgrade-changelog/SKILL.md (1)
43-45:⚠️ Potential issue | 🟡 MinorTag the fenced block language to pass MD040.
At Line 43, add a language identifier (e.g.,
text) to the opening fence.Suggested fix
- ``` + ```text Add new upgrades after this line based on the template above ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/upgrade-changelog/SKILL.md around lines 43 - 45, The fenced code block opener currently has no language tag; update the opening fence (the line with ```) so it reads as a language-tagged fence (e.g., add "text" after the backticks) so the block becomes ```text to satisfy MD040; leave the closing ``` unchanged..agents/skills/guidelines/SKILL.md (1)
59-63:⚠️ Potential issue | 🟡 MinorAdd a fence language to satisfy markdownlint MD040.
At Line 59, the fenced block is still untyped. Please annotate it (e.g.,
text) to avoid lint failures.Suggested fix
-``` +```text 1. [Step] → verify: [check] 2. [Step] → verify: [check] 3. [Step] → verify: [check]</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/guidelines/SKILL.md around lines 59 - 63, The fenced code
block containing the three checklist lines is untyped and triggers markdownlint
MD040; update that backtick block in SKILL.md (the block starting with "1.
[Step] → verify: [check]") by adding a fence language (for example "text") after
the openingso it becomestext, ensuring the block remains otherwise
unchanged.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>upgrades/software/v2.1.0/market.go (1)</summary><blockquote> `22-22`: **Fix misleading comment.** The comment says "marketMigrations deployment" which appears to be a copy-paste artifact. It should describe this as a market migration. <details> <summary>📝 Proposed fix</summary> ```diff -// handler marketMigrations deployment from version 7 to 8. +// handler migrates market module from version 7 to 8. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@upgrades/software/v2.1.0/market.go` at line 22, Update the misleading top-line comment in market.go that currently reads "handler marketMigrations deployment from version 7 to 8." to accurately describe the code as a market migration handler (e.g., "handler for market migration from version 7 to 8") so readers know this is a migration handler; locate the comment near the marketMigrations function and replace the copy-paste wording with the corrected phrase. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@testutil/network/util.go:
- Around line 51-53: The test mutates global discovery state via
aclient.SetRegistry(...) which can cause cross-test interference with
LocalRPCClient.Akash; instead, stop mutating global state by creating a local
registry instance with aclient.DefaultRegistry(aclient.WithChainID(cfg.ChainID))
and use that registry for the client(s) you construct (e.g., pass the registry
into the client constructor or aclient.New* variant that accepts a registry) or,
if the client API lacks that overload, capture the current registry, call
SetRegistry only locally during client creation and immediately restore the
original registry; update code references to
aclient.SetRegistry/aclient.DefaultRegistry and LocalRPCClient.Akash
accordingly.- Around line 31-32: The test registry initialization omits the node version,
making discovery/negotiation differ from production; update the registry
creation call in testutil/network/util.go (the code that currently initializes
the registry with just the chain ID) to include the node version option by
passing aclient.WithNodeVersion(version.Version) into the same registry
constructor call, and add the necessary import for the version package so the
tests mirror production's use of aclient.WithNodeVersion(version.Version).In
@upgrades/software/v2.1.0/market.go:
- Around line 14-16: newMarketMigration currently constructs and returns
deploymentMigrations{Migrator: m} which causes
deploymentMigrations.handler()/GetHandler() to be used instead of the intended
marketMigrations; fix by returning marketMigrations{Migrator: m} from
newMarketMigration so the marketMigrations type (and its GetHandler/handler
methods) are actually invoked; update the constructor in the newMarketMigration
function to instantiate marketMigrations with the provided Migrator.
Duplicate comments:
In @.agents/skills/guidelines/SKILL.md:
- Around line 59-63: The fenced code block containing the three checklist lines
is untyped and triggers markdownlint MD040; update that backtick block in
SKILL.md (the block starting with "1. [Step] → verify: [check]") by adding a
fence language (for example "text") after the openingso it becomestext,
ensuring the block remains otherwise unchanged.In @.agents/skills/upgrade-changelog/SKILL.md:
- Around line 43-45: The fenced code block opener currently has no language tag;
update the opening fence (the line with) so it reads as a language-tagged fence (e.g., add "text" after the backticks) so the block becomestext to
satisfy MD040; leave the closing ``` unchanged.
Nitpick comments:
In@upgrades/software/v2.1.0/market.go:
- Line 22: Update the misleading top-line comment in market.go that currently
reads "handler marketMigrations deployment from version 7 to 8." to accurately
describe the code as a market migration handler (e.g., "handler for market
migration from version 7 to 8") so readers know this is a migration handler;
locate the comment near the marketMigrations function and replace the copy-paste
wording with the corrected phrase.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `1a61674f-59fa-42c7-bf5a-733fdc7b5694` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 27e6142433a9af2708a7a083c9161508aa120279 and 05de3d2f8bb9a640a618f05bbe89db15883faa62. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (24)</summary> * `.agents/skills/guidelines/SKILL.md` * `.agents/skills/upgrade-changelog/SKILL.md` * `app/app.go` * `cmd/akash/cmd/app_creator.go` * `go.mod` * `tests/upgrade/docker-compose.hermes.yaml` * `testutil/network/rpc.go` * `testutil/network/util.go` * `upgrades/software/v2.1.0/init.go` * `upgrades/software/v2.1.0/market.go` * `upgrades/software/v2.1.0/upgrade.go` * `x/deployment/handler/server.go` * `x/deployment/imports/keepers.go` * `x/market/handler/handler.go` * `x/market/handler/handler_reclamation_test.go` * `x/market/handler/handler_test.go` * `x/market/handler/server.go` * `x/market/keeper/grpc_query.go` * `x/market/keeper/keeper.go` * `x/market/keeper/keeper_reclamation_test.go` * `x/market/keeper/keeper_test.go` * `x/market/module.go` * `x/market/simulation/genesis.go` * `x/oracle/keeper/abci.go` </details> <details> <summary>✅ Files skipped from review due to trivial changes (5)</summary> * cmd/akash/cmd/app_creator.go * testutil/network/rpc.go * tests/upgrade/docker-compose.hermes.yaml * x/market/handler/handler_test.go * go.mod </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (7)</summary> * x/market/module.go * x/market/keeper/grpc_query.go * x/market/simulation/genesis.go * x/market/handler/handler.go * x/oracle/keeper/abci.go * x/market/keeper/keeper_reclamation_test.go * x/market/handler/server.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+31
to
32
| aclient "pkg.akt.dev/go/node/client" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify registry initialization parity between app bootstrap and in-process test bootstrap.
rg -nP 'SetRegistry\\(aclient\\.DefaultRegistry\\(' testutil/network/util.go cmd/akash/cmd/app_creator.go
rg -nP 'WithNodeVersion\\(' testutil/network/util.go cmd/akash/cmd/app_creator.go
rg -nP 'func \\(c \\*LocalRPCClient\\) Akash\\(' testutil/network/rpc.goRepository: akash-network/node
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Search for SetRegistry calls with simpler patterns
echo "=== SetRegistry calls ==="
rg "SetRegistry" testutil/network/util.go cmd/akash/cmd/app_creator.go -n -A 3
echo -e "\n=== WithNodeVersion calls ==="
rg "WithNodeVersion" testutil/network/util.go cmd/akash/cmd/app_creator.go -n
echo -e "\n=== LocalRPCClient Akash method ==="
rg "func.*Akash" testutil/network/rpc.go -n -A 5Repository: akash-network/node
Length of output: 820
🏁 Script executed:
#!/bin/bash
# Check if version package is imported elsewhere in the codebase
echo "=== version package imports ==="
rg "github.com/cosmos/cosmos-sdk/version" --type go -l | head -5
echo -e "\n=== Akash client imports in cmd/akash ==="
rg "^import" cmd/akash/cmd/app_creator.go -A 15
echo -e "\n=== Current imports in testutil/network/util.go ==="
rg "^import" testutil/network/util.go -A 20Repository: akash-network/node
Length of output: 1188
🏁 Script executed:
#!/bin/bash
# Find the exact version import in app_creator.go
echo "=== Full imports in app_creator.go to find version ==="
sed -n '/^import/,/^)/p' cmd/akash/cmd/app_creator.go
echo -e "\n=== Check if there are any other SetRegistry patterns in the codebase ==="
rg "SetRegistry\|WithChainID\|WithNodeVersion" --type go -n | grep -v node_modulesRepository: akash-network/node
Length of output: 871
🏁 Script executed:
#!/bin/bash
# Verify exact line numbers and context around SetRegistry in both files
echo "=== testutil/network/util.go full context around line 51 ==="
sed -n '48,56p' testutil/network/util.go
echo -e "\n=== cmd/akash/cmd/app_creator.go full context around line 90 ==="
sed -n '87,95p' cmd/akash/cmd/app_creator.go
echo -e "\n=== Check current version module import in testutil/network/util.go ==="
grep "cosmos-sdk/version" testutil/network/util.go || echo "NOT FOUND - version import missing in testutil"
echo -e "\n=== Verify aclient package is imported in testutil/network/util.go ==="
rg "aclient\s+" testutil/network/util.go -n | head -5Repository: akash-network/node
Length of output: 1113
Add node version to test registry initialization to match production behavior.
Line 51 initializes the registry with chain ID only. Production bootstrap (cmd/akash/cmd/app_creator.go:90-93) includes node version via aclient.WithNodeVersion(version.Version), which is required for the version discovery registry used by the CometBFT JSON-RPC "akash" route and gRPC Discovery service. The test omission can make discovery and version-negotiation behavior differ from production.
Suggested fix
+import "github.com/cosmos/cosmos-sdk/version"
import (
aclient "pkg.akt.dev/go/node/client"
)
@@
aclient.SetRegistry(aclient.DefaultRegistry(
aclient.WithChainID(cfg.ChainID),
+ aclient.WithNodeVersion(version.Version),
))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testutil/network/util.go` around lines 31 - 32, The test registry
initialization omits the node version, making discovery/negotiation differ from
production; update the registry creation call in testutil/network/util.go (the
code that currently initializes the registry with just the chain ID) to include
the node version option by passing aclient.WithNodeVersion(version.Version) into
the same registry constructor call, and add the necessary import for the version
package so the tests mirror production's use of
aclient.WithNodeVersion(version.Version).
Comment on lines
+51
to
+53
| aclient.SetRegistry(aclient.DefaultRegistry( | ||
| aclient.WithChainID(cfg.ChainID), | ||
| )) |
There was a problem hiding this comment.
Process-global registry mutation can cause cross-test interference.
aclient.SetRegistry(...) mutates global state that is read by LocalRPCClient.Akash. Parallel in-process networks can overwrite each other’s discovery metadata and produce flaky tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testutil/network/util.go` around lines 51 - 53, The test mutates global
discovery state via aclient.SetRegistry(...) which can cause cross-test
interference with LocalRPCClient.Akash; instead, stop mutating global state by
creating a local registry instance with
aclient.DefaultRegistry(aclient.WithChainID(cfg.ChainID)) and use that registry
for the client(s) you construct (e.g., pass the registry into the client
constructor or aclient.New* variant that accepts a registry) or, if the client
API lacks that overload, capture the current registry, call SetRegistry only
locally during client creation and immediately restore the original registry;
update code references to aclient.SetRegistry/aclient.DefaultRegistry and
LocalRPCClient.Akash accordingly.
Comment on lines
+14
to
+16
| func newMarketMigration(m utypes.Migrator) utypes.Migration { | ||
| return deploymentMigrations{Migrator: m} | ||
| } |
There was a problem hiding this comment.
Critical: Constructor returns wrong type.
newMarketMigration returns deploymentMigrations{...} instead of marketMigrations{...}. This means the marketMigrations.GetHandler() and marketMigrations.handler() methods will never be invoked—the migration will use deploymentMigrations's handler instead.
🐛 Proposed fix
func newMarketMigration(m utypes.Migrator) utypes.Migration {
- return deploymentMigrations{Migrator: m}
+ return marketMigrations{Migrator: m}
}📝 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
| func newMarketMigration(m utypes.Migrator) utypes.Migration { | |
| return deploymentMigrations{Migrator: m} | |
| } | |
| func newMarketMigration(m utypes.Migrator) utypes.Migration { | |
| return marketMigrations{Migrator: m} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@upgrades/software/v2.1.0/market.go` around lines 14 - 16, newMarketMigration
currently constructs and returns deploymentMigrations{Migrator: m} which causes
deploymentMigrations.handler()/GetHandler() to be used instead of the intended
marketMigrations; fix by returning marketMigrations{Migrator: m} from
newMarketMigration so the marketMigrations type (and its GetHandler/handler
methods) are actually invoked; update the constructor in the newMarketMigration
function to instantiate marketMigrations with the provided Migrator.
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.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md