feat: rewards utility distribution - #367
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe rewards module replaces timed release schedules with bonded-ratio-driven inflation emissions. Governance controls inflation bounds, bonding goals, supply base, and denomination. The reward pool tracks release time and cumulative releases. Schedule queries, transactions, storage, validation, and CLI commands were removed. Begin-block processing now calculates, caps, transfers, and records rewards. Tests, documentation, coverage, and a manual smoke test were updated. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/e2e/e2e_rewards_test.go (1)
174-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
depositAmountin the proposal body.The embedded deposit is a literal
1000akii, whilepassRewardsParamsProposaldepositsdepositAmountin a separate step. If the chain minimum deposit changes, the two values drift. Format the body withdepositAmount.String().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/e2e_rewards_test.go` at line 174, Update the proposal body construction in passRewardsParamsProposal to use depositAmount.String() for the embedded deposit value instead of the hardcoded "1000akii" literal, keeping the body and separate deposit step synchronized.x/rewards/README.md (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code block.
markdownlint reports MD040 for this block.
📝 Proposed fix
-``` +```text inflation = clamp((1 - bondedRatio/goalBonded) × 0.13 × bondedRatio, inflationMin, inflationMax) amount = inflation × supplyBase × elapsedNs / nsPerYear pay = min(amount, poolBalance)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@x/rewards/README.mdaround lines 16 - 20, Add the text language identifier
to the fenced code block containing the inflation, amount, and pay formulas in
the README, preserving the existing content and formatting.</details> <!-- cr-comment:v1:2e80bc9deb847036e31c828a --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>x/rewards/keeper/abci_test.go (1)</summary><blockquote> `136-138`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **The conditional `Skip` can disable the main assertions.** The expected amount depends on the bonded ratio of the test app. If that ratio reaches or exceeds `GoalBonded`, `CalculateInflation` clamps the rate to `InflationMin` (`0` by default), the expected coin is zero, and the "normal distribution" case skips every transfer assertion. The test then passes without checking any emission. Set `InflationMin` to a positive value for the test parameters, or assert that the calculated amount is positive instead of skipping. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@x/rewards/keeper/abci_test.goaround lines 136 - 138, The normal
distribution test must not skip its transfer assertions when CalculateInflation
returns a zero expected amount. Update the test setup parameters to use a
positive InflationMin, or replace the conditional Skip around expected with a
positive-amount assertion, while preserving the transfer assertions in the
normal distribution case.</details> <!-- cr-comment:v1:cc68c5c2c774f2bf0e4f0eee --> </blockquote></details> <details> <summary>x/rewards/types/reward.go (1)</summary><blockquote> `42-53`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Consider dropping the unused `error` return.** `CalculateReward` returns `nil` on every path. The `error` result forces dead error handling in `BeginBlocker` and in the tests. Return only the coin and the inflation rate, or return a real error for invalid input. Also applies to: 64-73 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@x/rewards/types/reward.goaround lines 42 - 53, Remove the unused error
return from CalculateReward and update all callers, including BeginBlocker and
tests, to handle only the coin and inflation-rate results. Keep the existing
reward calculations and zero-supply behavior unchanged; do not retain dead nil
error handling.</details> <!-- cr-comment:v1:2cebeb356c08aa9c8edf78b7 --> </blockquote></details> <details> <summary>contrib/scripts/test_rewards_manual.sh (1)</summary><blockquote> `157-159`: _🩺 Stability & Availability_ | _🔵 Trivial_ | _💤 Low value_ **The height retry loop exits on a transient RPC failure.** `set -e` with `pipefail` is active. If `curl -sf` or `jq` fails inside the command substitution, the assignment fails and the script exits instead of retrying. Tolerate the failure so the loop can retry. <details> <summary>♻️ Proposed change</summary> ```diff - height="$(curl -sf "$RPC_URL/status" | jq -r '.result.sync_info.latest_block_height')" + height="$(curl -sf "$RPC_URL/status" | jq -r '.result.sync_info.latest_block_height' || true)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/scripts/test_rewards_manual.sh` around lines 157 - 159, Update the height assignment in the retry loop to tolerate transient failures from curl or jq under set -e and pipefail, allowing the loop to continue when the RPC request or parsing fails. Preserve the existing numeric validation and height > 1 success condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-13: In the Unreleased section of CHANGELOG.md, remove or merge
the entries describing haltSchedule, schedule durations, and EndTime fixes,
since ReleaseSchedule mechanics were removed. Keep the new inflation-based
rewards entry and unrelated release notes unchanged.
In `@contrib/scripts/test_rewards_manual.sh`:
- Around line 81-82: Add a need_cmd python3 prerequisite check alongside the
existing need_cmd jq and need_cmd curl checks in the script’s initial
prerequisite section, so the script validates Python availability before
building or starting the chain.
In `@proto/kiichain/rewards/v1beta1/params.proto`:
- Around line 5-36: Fix the shared Buf module dependency and import
configuration so gogoproto extensions, Cosmos Coin, and cosmos.msg.v1.service
resolve. In proto/kiichain/rewards/v1beta1/params.proto lines 5-36, verify
gogoproto/gogo.proto resolves; in types.proto lines 12-31, verify gogoproto and
Coin, adding Coin’s explicit import only if still needed; in genesis.proto lines
15-21, recheck RewardPool after types.proto compiles with no direct change if
the error was cascading; and in tx.proto lines 13-25, correct the
cosmos.msg.v1.service dependency/import path. Re-run buf build and buf lint for
all four files.
---
Nitpick comments:
In `@contrib/scripts/test_rewards_manual.sh`:
- Around line 157-159: Update the height assignment in the retry loop to
tolerate transient failures from curl or jq under set -e and pipefail, allowing
the loop to continue when the RPC request or parsing fails. Preserve the
existing numeric validation and height > 1 success condition.
In `@tests/e2e/e2e_rewards_test.go`:
- Line 174: Update the proposal body construction in passRewardsParamsProposal
to use depositAmount.String() for the embedded deposit value instead of the
hardcoded "1000akii" literal, keeping the body and separate deposit step
synchronized.
In `@x/rewards/keeper/abci_test.go`:
- Around line 136-138: The normal distribution test must not skip its transfer
assertions when CalculateInflation returns a zero expected amount. Update the
test setup parameters to use a positive InflationMin, or replace the conditional
Skip around expected with a positive-amount assertion, while preserving the
transfer assertions in the normal distribution case.
In `@x/rewards/README.md`:
- Around line 16-20: Add the text language identifier to the fenced code block
containing the inflation, amount, and pay formulas in the README, preserving the
existing content and formatting.
In `@x/rewards/types/reward.go`:
- Around line 42-53: Remove the unused error return from CalculateReward and
update all callers, including BeginBlocker and tests, to handle only the coin
and inflation-rate results. Keep the existing reward calculations and
zero-supply behavior unchanged; do not retain dead nil error handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bb3fbc6e-7cb2-483f-a302-fd605e0fb7f0
⛔ Files ignored due to path filters (6)
x/rewards/types/genesis.pb.gois excluded by!**/*.pb.gox/rewards/types/params.pb.gois excluded by!**/*.pb.gox/rewards/types/query.pb.gois excluded by!**/*.pb.gox/rewards/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/rewards/types/tx.pb.gois excluded by!**/*.pb.gox/rewards/types/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (39)
CHANGELOG.mdapp/keepers/keepers.gocontrib/scripts/test_rewards_manual.shproto/kiichain/rewards/v1beta1/genesis.protoproto/kiichain/rewards/v1beta1/params.protoproto/kiichain/rewards/v1beta1/query.protoproto/kiichain/rewards/v1beta1/tx.protoproto/kiichain/rewards/v1beta1/types.prototests/e2e/e2e_rest_regression_test.gotests/e2e/e2e_rewards_test.gotests/e2e/e2e_setup_test.gox/rewards/README.mdx/rewards/client/cli/query.gox/rewards/client/cli/tx.gox/rewards/keeper/abci.gox/rewards/keeper/abci_test.gox/rewards/keeper/genesis.gox/rewards/keeper/grpc_query.gox/rewards/keeper/grpc_query_test.gox/rewards/keeper/keeper.gox/rewards/keeper/msg_server.gox/rewards/keeper/msg_server_test.gox/rewards/keeper/validation.gox/rewards/module.gox/rewards/types/codec.gox/rewards/types/codec_test.gox/rewards/types/events.gox/rewards/types/expected_keepers.gox/rewards/types/genesis.gox/rewards/types/genesis_test.gox/rewards/types/keys.gox/rewards/types/msg.gox/rewards/types/params.gox/rewards/types/params_test.gox/rewards/types/release_schedule.gox/rewards/types/release_schedule_test.gox/rewards/types/reward.gox/rewards/types/reward_pool.gox/rewards/types/reward_test.go
💤 Files with no reviewable changes (9)
- x/rewards/keeper/grpc_query.go
- x/rewards/types/codec.go
- x/rewards/client/cli/query.go
- x/rewards/keeper/validation.go
- x/rewards/client/cli/tx.go
- x/rewards/types/release_schedule_test.go
- proto/kiichain/rewards/v1beta1/query.proto
- x/rewards/types/release_schedule.go
- x/rewards/types/msg.go
d3069f6 to
dab9771
Compare
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)
x/rewards/keeper/abci_test.go (2)
170-185: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert all state remains unchanged on no-transfer paths.
The early-return branches do not check
pool.TotalReleasedor the fee-collector balance. A regression can update cumulative accounting or transfer coins while these cases still pass. Before each return, comparepool.TotalReleasedwithtc.initialPool.TotalReleasedand compare the current fee-collector balance withinitialFeeCollectorBalance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/rewards/keeper/abci_test.go` around lines 170 - 185, Update the early-return branches in the test around expectLastReleaseSet, expectLastReleaseAdv, and !expectTransfer to also assert pool.TotalReleased equals tc.initialPool.TotalReleased and the current fee-collector balance equals initialFeeCollectorBalance before returning.
126-136: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not skip distribution assertions for a live bonded ratio.
When
expected.IsZero(),suite.T().Skipexits afterBeginBlockerwithout checking pool deduction,LastReleaseTime,TotalReleased, or the fee-collector transfer. This makes coverage depend on the staking fixture. Configure a deterministic bonded ratio and make zero output an explicit test case. Also ensure the pool-depletion case proves that the uncapped one-year reward exceeds 10 coins before expecting the full remaining balance.Also applies to: 188-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/rewards/keeper/abci_test.go` around lines 126 - 136, Update the reward distribution tests around BeginBlocker to use a deterministic live bonded ratio instead of skipping when expected.IsZero, and make zero output an explicit case while still asserting pool deduction, LastReleaseTime, TotalReleased, and fee-collector transfer. In the “pool runs dry - pays remaining balance” case, configure the fixture so the uncapped one-year reward is provably greater than 10 coins before asserting the pool pays its full remaining balance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@x/rewards/keeper/abci_test.go`:
- Around line 104-123: Update TestBeginBlocker so each table-driven case resets
or funds the rewards module account before calling RewardPool.Set, ensuring the
actual bank balance matches the case’s configured CommunityPool; preserve the
existing assertions and use the established module-account funding path rather
than relying on shared suite.App state.
---
Outside diff comments:
In `@x/rewards/keeper/abci_test.go`:
- Around line 170-185: Update the early-return branches in the test around
expectLastReleaseSet, expectLastReleaseAdv, and !expectTransfer to also assert
pool.TotalReleased equals tc.initialPool.TotalReleased and the current
fee-collector balance equals initialFeeCollectorBalance before returning.
- Around line 126-136: Update the reward distribution tests around BeginBlocker
to use a deterministic live bonded ratio instead of skipping when
expected.IsZero, and make zero output an explicit case while still asserting
pool deduction, LastReleaseTime, TotalReleased, and fee-collector transfer. In
the “pool runs dry - pays remaining balance” case, configure the fixture so the
uncapped one-year reward is provably greater than 10 coins before asserting the
pool pays its full remaining balance.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c587a80-65d6-4139-9534-ce89bfbcbb9f
📒 Files selected for processing (8)
.github/workflows/test.ymlCHANGELOG.mdcontrib/scripts/test_rewards_manual.shx/rewards/keeper/abci.gox/rewards/keeper/abci_test.gox/rewards/types/reward.gox/rewards/types/reward_pool_test.gox/rewards/types/reward_test.go
💤 Files with no reviewable changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/test.yml
- x/rewards/types/reward_test.go
- x/rewards/types/reward_pool_test.go
- x/rewards/types/reward.go
- contrib/scripts/test_rewards_manual.sh
- x/rewards/keeper/abci.go
There was a problem hiding this comment.
Pull request overview
This PR redesigns x/rewards to replace the legacy timed linear ReleaseSchedule with continuous, inflation-curve-based emissions driven by staking bonded ratio. Emissions remain prefunded (pool → fee_collector → distribution) and are enabled/disabled via governance by setting supply_base (0 = off). It also bumps the module consensus version to 2 and removes the schedule-related state/API/CLI/query surface.
Changes:
- Remove
ReleaseSchedulestate +MsgChangeSchedule+ release-schedule query/CLI; migrate to inflation-curve emissions withRewardPool.last_release_timeandRewardPool.total_released. - Add new governance params (
goal_bonded,inflation_min,inflation_max,supply_base) and stakingBondedRatiodependency to compute per-block release. - Update tests/E2E, docs, protobufs, and CI coverage to reflect the new rewards distribution model.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| x/rewards/types/types.pb.go | Removes ReleaseSchedule protobuf type; adds RewardPool.last_release_time and total_released. |
| x/rewards/types/tx.pb.go | Removes MsgChangeSchedule from tx service/types. |
| x/rewards/types/reward.go | Introduces inflation-curve math and time-proportional per-block reward calculation. |
| x/rewards/types/reward_test.go | Replaces schedule tests with inflation/reward tests. |
| x/rewards/types/reward_pool.go | Extends RewardPool genesis/init/validation with new fields. |
| x/rewards/types/reward_pool_test.go | Adds genesis validation coverage for TotalReleased. |
| x/rewards/types/release_schedule.go | Deletes schedule state helpers. |
| x/rewards/types/release_schedule_test.go | Deletes schedule genesis tests. |
| x/rewards/types/query.pb.gw.go | Removes REST gateway handlers for release-schedule. |
| x/rewards/types/query.pb.go | Removes gRPC query for ReleaseSchedule. |
| x/rewards/types/params.pb.go | Adds new params fields to protobuf-gen code. |
| x/rewards/types/params.go | Adds defaults and validation for new inflation params. |
| x/rewards/types/params_test.go | Updates params validation tests for new fields. |
| x/rewards/types/msg.go | Removes NewMsgChangeSchedule and message registration. |
| x/rewards/types/keys.go | Removes schedule store prefix key. |
| x/rewards/types/genesis.pb.go | Removes schedule from genesis protobuf type. |
| x/rewards/types/genesis.go | Updates genesis constructors/validation for new state shape. |
| x/rewards/types/genesis_test.go | Updates genesis tests to remove schedule and validate new fields. |
| x/rewards/types/expected_keepers.go | Adds StakingKeeper interface for bonded ratio dependency. |
| x/rewards/types/events.go | Removes schedule event keys; adds inflation/bonded-ratio attributes. |
| x/rewards/types/codec.go | Removes schedule msg from interface registrations. |
| x/rewards/types/codec_test.go | Updates interface registration expectations. |
| x/rewards/README.md | Updates module documentation to inflation-based design and formula. |
| x/rewards/module.go | Bumps module ConsensusVersion to 2. |
| x/rewards/keeper/validation.go | Removes schedule validation helpers. |
| x/rewards/keeper/msg_server.go | Removes ChangeSchedule msg handler and related logic. |
| x/rewards/keeper/msg_server_test.go | Removes schedule msg server tests; updates params tests. |
| x/rewards/keeper/keeper.go | Adds staking keeper dependency; removes schedule collections item. |
| x/rewards/keeper/grpc_query.go | Removes ReleaseSchedule gRPC query implementation. |
| x/rewards/keeper/grpc_query_test.go | Removes schedule query tests; updates params/pool tests. |
| x/rewards/keeper/genesis.go | Removes schedule init/export from keeper genesis flows. |
| x/rewards/keeper/abci.go | Replaces schedule-based emissions with inflation-based BeginBlocker logic. |
| x/rewards/keeper/abci_test.go | Updates BeginBlocker tests and adds init/export coverage for new state. |
| x/rewards/client/cli/tx.go | Removes change-schedule tx CLI command. |
| x/rewards/client/cli/query.go | Removes release-schedule query CLI command. |
| tests/e2e/e2e_setup_test.go | Renames proposal fixture for rewards params update. |
| tests/e2e/e2e_rewards_test.go | Updates E2E to enable emissions via MsgUpdateParams and assert pool drain/rewards increase. |
| tests/e2e/e2e_rest_regression_test.go | Removes REST regression check for /release-schedule. |
| proto/kiichain/rewards/v1beta1/types.proto | Removes ReleaseSchedule; adds new RewardPool fields. |
| proto/kiichain/rewards/v1beta1/tx.proto | Removes ChangeSchedule RPC/message; updates service comments. |
| proto/kiichain/rewards/v1beta1/query.proto | Removes ReleaseSchedule query/messages. |
| proto/kiichain/rewards/v1beta1/params.proto | Adds new params fields (goal_bonded, min/max, supply_base). |
| proto/kiichain/rewards/v1beta1/genesis.proto | Reserves former release_schedule field number/name. |
| contrib/scripts/test_rewards_manual.sh | Adds manual smoke test script for the new emissions path. |
| CHANGELOG.md | Documents replacement of schedule with bonded-ratio inflation emissions and new params/state. |
| app/keepers/keepers.go | Wires staking keeper into rewards keeper constructor. |
| .github/workflows/test.yml | Adds x/rewards coverage artifact and Codecov upload entry. |
Suppressed comments (2)
x/rewards/keeper/abci.go:72
- If SendCoinsFromModuleToModule fails, BeginBlocker logs and returns nil without updating rewardPool.LastReleaseTime. This can accumulate Δt and cause a later block (when the bank error clears) to emit a large catch-up amount (capped by poolBalance), potentially draining the pool unexpectedly. Persisting LastReleaseTime on this path would keep emissions proportional to real time even across transient transfer failures.
if err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, k.feeCollectorName, coinsToDistribute); err != nil {
k.Logger(ctx).Error("failed to send rewards to fee collector", "error", err)
return nil
}
x/rewards/keeper/abci.go:79
- When CommunityPool.SafeSub indicates a negative result, BeginBlocker logs and returns nil without advancing rewardPool.LastReleaseTime. That can also accrue Δt and lead to a large catch-up emission if/when the accounting issue is resolved. Advancing LastReleaseTime here keeps the module from trying to "catch up" and dump the pool after a transient divergence.
remaining, hasNeg := rewardPool.CommunityPool.SafeSub(sdk.NewDecCoinsFromCoins(coinsToDistribute...))
if hasNeg {
k.Logger(ctx).Error("community pool subtraction resulted in negative balance",
"denom", amountToDistribute.Denom)
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
contrib/scripts/e2e_gov_module_block_deposit.sh (3)
62-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the remaining hard dependencies to the
needchecks.The script also requires
shasum(lines 164, 538),lsof(lines 235, 240),pkill(lines 77, 231), andmake(line 160). Without these checks, a missing tool produces a late and unclear failure.shasumandlsofare absent on several minimal Linux images.♻️ Proposed fix
need jq need cast need go need sed need python3 +need make +need shasum +need lsof +need pkill🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/scripts/e2e_gov_module_block_deposit.sh` around lines 62 - 66, Extend the dependency checks at the top of the script to include shasum, lsof, pkill, and make alongside the existing need entries. Keep the checks as required commands so the script fails early when any tool used by the script is unavailable.
86-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused helpers and variables.
gov_is_unblocked_in_tree(lines 86-88) andbech32_to_hex(lines 379-389) are never called.MIN_DEPOSIT(line 36) andAPI_PORT(line 46) are never referenced; the genesis edit hardcodes2000000000000000000at lines 277-278 instead. Delete the dead code, or useMIN_DEPOSITin thejqfilter so the deposit constant exists in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/scripts/e2e_gov_module_block_deposit.sh` around lines 86 - 88, Remove the unused gov_is_unblocked_in_tree and bech32_to_hex helpers, along with unreferenced MIN_DEPOSIT and API_PORT variables; alternatively, replace the hardcoded deposit value in the jq genesis-edit filter with MIN_DEPOSIT and retain that variable as the single source of truth.Source: Linters/SAST tools
233-246: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePort-based kill can terminate an unrelated process.
stop_nodekills every PID that listens on the four ports, includingkill -9. The ports are configurable through the environment, so a developer who setsRPC_PORTto a port used by another service loses that process. Restrict the kill to descendants of this script, or verify the command line before you kill.♻️ Proposed fix: only kill processes whose command line matches this test node
pids="$(lsof -tiTCP:"${port}" -sTCP:LISTEN 2>/dev/null || true)" if [[ -n "${pids}" ]]; then - # shellcheck disable=SC2086 - kill ${pids} 2>/dev/null || true + local pid + for pid in ${pids}; do + if ps -p "${pid}" -o command= 2>/dev/null | grep -q "start --home ${WORKDIR}"; then + kill "${pid}" 2>/dev/null || true + fi + done sleep 0.5🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/scripts/e2e_gov_module_block_deposit.sh` around lines 233 - 246, Update stop_node’s port cleanup loop to avoid killing unrelated listeners when configurable ports overlap other services. Before both the graceful kill and the kill -9 fallback, verify each PID belongs to the test node started by this script, using its process ancestry or command line; skip any PID that does not match, while preserving cleanup for the intended node processes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contrib/scripts/e2e_gov_module_block_deposit.sh`:
- Line 69: Update the die() function to print the fatal error message only once,
removing the duplicate stdout printf while preserving the stderr output and exit
behavior.
- Around line 136-169: Update build_variant to register restoration of APP_GO
with the existing cleanup mechanism before any call to ensure_govtypes_import or
write_blocked_fn, so failures during patching, make build, or copying restore
the exact original content. Remove the git checkout invocation after mv,
preserving unrelated uncommitted changes while retaining normal successful-build
behavior.
---
Nitpick comments:
In `@contrib/scripts/e2e_gov_module_block_deposit.sh`:
- Around line 62-66: Extend the dependency checks at the top of the script to
include shasum, lsof, pkill, and make alongside the existing need entries. Keep
the checks as required commands so the script fails early when any tool used by
the script is unavailable.
- Around line 86-88: Remove the unused gov_is_unblocked_in_tree and
bech32_to_hex helpers, along with unreferenced MIN_DEPOSIT and API_PORT
variables; alternatively, replace the hardcoded deposit value in the jq
genesis-edit filter with MIN_DEPOSIT and retain that variable as the single
source of truth.
- Around line 233-246: Update stop_node’s port cleanup loop to avoid killing
unrelated listeners when configurable ports overlap other services. Before both
the graceful kill and the kill -9 fallback, verify each PID belongs to the test
node started by this script, using its process ancestry or command line; skip
any PID that does not match, while preserving cleanup for the intended node
processes.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ad22138-14d2-4c91-954b-d66218b74ffc
📒 Files selected for processing (3)
contrib/scripts/e2e_gov_module_block_deposit.shx/rewards/keeper/abci.gox/rewards/keeper/abci_error_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/rewards/keeper/abci.go
Description
Replaces the timed linear
ReleaseScheduleinx/rewardswith continuous inflation-based emissions driven by bonded ratio.Rewards still come from the prefunded pool →
fee_collector→x/distributionpath (begin-block order unchanged). Per-block amount is now:clamp((1 − bonded/goal) × 0.13 × bonded, min, max) × supply_base × Δt / yearcapped at remaining pool balance (emits until the pool runs dry).
Removed:
ReleaseSchedule,MsgChangeSchedule, end-date / total-budget schedule semantics, and the release-schedule query/CLI.Added: staking
BondedRatiodependency; gov paramsgoal_bonded,inflation_min,inflation_max,supply_base(default0= off); hardcodedinflation_rate_change = 0.13;last_release_time/total_releasedonRewardPool; richerreward_distributedevents (inflation_rate,bonded_ratio).Enable path: fund pool (
MsgFundPool) → govMsgUpdateParamswithsupply_base > 0. Disable: setsupply_baseto0.Consensus version bumped to
2. No new external dependencies.Type of change
How Has This Been Tested?
go test -tags=test ./x/rewards/...(curve math, BeginBlocker, msgs, queries, params, genesis)go test -tags=test ./x/rewards/types -run 'TestCalculate'and./x/rewards/keeper -run 'TestKeeperTestSuite/TestBeginBlocker'SKIP_IBC_TESTS=true go test -tags=test ./tests/e2e -run 'TestIntegrationTestSuite/TestRewards'(fund pool → gov setsupply_base→ pool decreases / validator rewards increase)./contrib/scripts/test_rewards_manual.sh(fund → gov setsupply_base→ pool decreases /total_releasedincreases)PR Checklist:
Make sure each step was done:
make lint-fix