style: low-risk lint fixes#1391
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThe Makefile adds revision-based lint targets. Several Go files update import aliases, replace fixed-string error creation with ChangesGo and Makefile updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
x/sequencer/keeper/replace_proposer.go (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse idiomatic nil-check ordering and lowercase static error text.
Suggested patch
- if nil == data { - return errors.New("SetReplaceProposer data is nil") + if data == nil { + return errors.New("set replace proposer data is nil") }As per path instructions, "
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations."🤖 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/sequencer/keeper/replace_proposer.go` around lines 15 - 16, The nil guard in replace_proposer.go should follow idiomatic Go style by checking the variable against nil in the usual order and by using lowercase static error text. Update the nil check in the SetReplaceProposer path to match Uber Go style conventions, and adjust the returned error string in the relevant function so it starts with a lowercase letter while keeping the existing message meaning.Source: Path instructions
🤖 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/sequencer/keeper/grpc_query_sequencers_by_rollapp.go`:
- Line 5: The gRPC handler in sequencersByRollapp should not return a generic
error; replace the current errors.New("unsupport function") return in the query
handler with a status error using codes.Unimplemented so clients receive the
correct gRPC status. Update the grpc_query_sequencers_by_rollapp.go handler to
use status.Error(codes.Unimplemented, "unsupported function") and ensure the
needed gRPC status/codes imports are present while removing the unused errors
import.
---
Nitpick comments:
In `@x/sequencer/keeper/replace_proposer.go`:
- Around line 15-16: The nil guard in replace_proposer.go should follow
idiomatic Go style by checking the variable against nil in the usual order and
by using lowercase static error text. Update the nil check in the
SetReplaceProposer path to match Uber Go style conventions, and adjust the
returned error string in the relevant function so it starts with a lowercase
letter while keeping the existing message meaning.
🪄 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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: fbd3432b-97ca-45d3-938e-48977c2046c7
📒 Files selected for processing (13)
Makefileapp/modules.gocmd/med/cmd/gen_relayer.gocmd/med/cmd/utils.gox/dao/keeper/msg_server.gox/delayedack/types/params.gox/denommetadata/ibc_middleware.gox/kyc/keeper/did.gox/kyc/keeper/keeper.gox/sequencer/keeper/grpc_query_sequencers_by_rollapp.gox/sequencer/keeper/replace_proposer.gox/sequencer/simulation/create_sequencer.gox/sequencer/types/genesis.go
|
|
||
| import ( | ||
| "context" | ||
| "errors" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,140p' x/sequencer/keeper/grpc_query_sequencers_by_rollapp.go
rg -n 'UnConfirmSequencerAddressByRollappByStatus|errors\.New\("unsupport function"\)|status\.Error|codes\.Unimplemented' x/sequencer/keeper/grpc_query_sequencers_by_rollapp.goRepository: openmetaearth/me-hub
Length of output: 5008
Return codes.Unimplemented from this handler. errors.New("unsupport function") is a generic error and will surface to clients as Unknown; use status.Error(codes.Unimplemented, "unsupported function") instead.
🤖 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/sequencer/keeper/grpc_query_sequencers_by_rollapp.go` at line 5, The gRPC
handler in sequencersByRollapp should not return a generic error; replace the
current errors.New("unsupport function") return in the query handler with a
status error using codes.Unimplemented so clients receive the correct gRPC
status. Update the grpc_query_sequencers_by_rollapp.go handler to use
status.Error(codes.Unimplemented, "unsupported function") and ensure the needed
gRPC status/codes imports are present while removing the unused errors import.
Source: Path instructions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/wstaking/keeper/msg_server_delegate.go`:
- Around line 70-75: The reward payout check in msg_server_delegate.go is using
a post-subtraction balance, which can wrongly reject valid payouts. Update the
logic in the delegate reward handling flow around region.DelegateInterest so the
insufficient-funds validation happens before subtracting rewards, and only
subtract after that check passes; use the existing region.DelegateInterest,
rewards, and the surrounding delegate payout code path to locate it.
🪄 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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: f3f25473-36a1-48ee-a9bc-908c3e670e92
📒 Files selected for processing (15)
app/apptesting/test_helpers.goapp/keepers/keys.goapp/keepers/wasm_me_msg.gocmd/med/cmd/genmoduleaccount.gox/wstaking/client/cli/tx_replace_consensus_pubkey.gox/wstaking/keeper/alias_functions.gox/wstaking/keeper/fixed_deposit_cfg.gox/wstaking/keeper/kyc_reward.gox/wstaking/keeper/msg_server.gox/wstaking/keeper/msg_server_delegate.gox/wstaking/keeper/msg_server_fixed_deposit.gox/wstaking/keeper/msg_server_ibc_transfer_from_region_treasure.gox/wstaking/keeper/msg_server_undelegate.gox/wstaking/keeper/msg_server_validator.gox/wstaking/keeper/update_pub_key.go
💤 Files with no reviewable changes (1)
- cmd/med/cmd/genmoduleaccount.go
✅ Files skipped from review due to trivial changes (6)
- x/wstaking/client/cli/tx_replace_consensus_pubkey.go
- app/keepers/wasm_me_msg.go
- x/wstaking/keeper/msg_server_validator.go
- x/wstaking/keeper/update_pub_key.go
- app/keepers/keys.go
- app/apptesting/test_helpers.go
Comment on lines
70
to
75
| if region.DelegateInterest.GTE(rewards) { | ||
| region.DelegateInterest = region.DelegateInterest.Sub(rewards) | ||
| } else { | ||
| } | ||
| if !region.DelegateInterest.GTE(rewards) { | ||
| return nil, fmt.Errorf("region(%s) total interest not enough.need pay %s,only have %s", | ||
| region.RegionId, rewards.String(), region.DelegateInterest.String()) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Post-subtraction check rejects valid reward payouts.
Line 73 re-checks region.DelegateInterest >= rewards after subtracting rewards, so any balance between rewards and 2*rewards now fails even though it was sufficient before the deduction. Keep the insufficiency test on the pre-subtraction balance.
🐛 Proposed fix
- if region.DelegateInterest.GTE(rewards) {
- region.DelegateInterest = region.DelegateInterest.Sub(rewards)
- }
- if !region.DelegateInterest.GTE(rewards) {
+ if !region.DelegateInterest.GTE(rewards) {
return nil, fmt.Errorf("region(%s) total interest not enough.need pay %s,only have %s",
region.RegionId, rewards.String(), region.DelegateInterest.String())
}
+ region.DelegateInterest = region.DelegateInterest.Sub(rewards)📝 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
| if region.DelegateInterest.GTE(rewards) { | |
| region.DelegateInterest = region.DelegateInterest.Sub(rewards) | |
| } else { | |
| } | |
| if !region.DelegateInterest.GTE(rewards) { | |
| return nil, fmt.Errorf("region(%s) total interest not enough.need pay %s,only have %s", | |
| region.RegionId, rewards.String(), region.DelegateInterest.String()) | |
| if !region.DelegateInterest.GTE(rewards) { | |
| return nil, fmt.Errorf("region(%s) total interest not enough.need pay %s,only have %s", | |
| region.RegionId, rewards.String(), region.DelegateInterest.String()) | |
| } | |
| region.DelegateInterest = region.DelegateInterest.Sub(rewards) |
🤖 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/wstaking/keeper/msg_server_delegate.go` around lines 70 - 75, The reward
payout check in msg_server_delegate.go is using a post-subtraction balance,
which can wrongly reject valid payouts. Update the logic in the delegate reward
handling flow around region.DelegateInterest so the insufficient-funds
validation happens before subtracting rewards, and only subtract after that
check passes; use the existing region.DelegateInterest, rewards, and the
surrounding delegate payout code path to locate it.
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.
Summary
Related Issues
Type of Change
featNew featurefixBug fixrefactorInternal refactordocsDocumentation onlytestTest-only changechoreBuild, tooling, or maintenanceValidation
make testmake lintValidation Notes
Checklist
fix: avoid batch block key collisions).protoor generated client files changedBreaking Changes / Migration Notes
Additional Context
Summary by CodeRabbit