Skip to content

Conversation

@ledigang
Copy link

@ledigang ledigang commented Nov 21, 2025

Changelist

There is a new function added in the go1.21 standard library, which can make the code more concise and easy to read.

Test Plan

[Describe how this PR was tested (if applicable)]

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

  • Refactor
    • Updated internal sorting utilities across multiple modules to use modern library functions, improving code maintainability. Existing functionality remains unchanged.

✏️ Tip: You can customize this high-level summary in your review settings.

@ledigang ledigang requested a review from a team as a code owner November 21, 2025 10:25
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

Walkthrough

The PR updates four Go source files to replace sort.Slice function calls with slices.Sort from the standard library's slices package, standardizing the sorting approach while preserving identical sorting behavior.

Changes

Cohort / File(s) Summary
Sort.Slice to slices.Sort migration
protocol/lib/math.go, protocol/x/clob/keeper/grpc_query_leverage.go, protocol/x/perpetuals/keeper/perpetual.go, protocol/x/prices/client/testutil/util.go
Replace sort.Slice calls with slices.Sort and update imports from sort to slices package. Maintains identical sorting behavior and deterministic ordering across all usages.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Homogeneous refactoring pattern applied consistently across all four files
  • No logic changes, no behavioral modifications
  • Simple import swaps and direct function call replacements
  • No exported API changes

Poem

🐰 Whiskers twitching with delight,
Sort to slices—what a sight!
Four files neat, now unified,
Modern Go with style, applied!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main refactoring change: replacing sort.Slice with slices.Sort across multiple files for better code clarity.
Description check ✅ Passed The description includes the required Changelist and Author/Reviewer Checklist sections. However, the Test Plan section is left incomplete with only the template placeholder text.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2121b2 and 4e8c407.

📒 Files selected for processing (4)
  • protocol/lib/math.go (2 hunks)
  • protocol/x/clob/keeper/grpc_query_leverage.go (2 hunks)
  • protocol/x/perpetuals/keeper/perpetual.go (2 hunks)
  • protocol/x/prices/client/testutil/util.go (2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-15T16:17:29.092Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/clob/types/expected_keepers.go:86-90
Timestamp: 2024-11-15T16:17:29.092Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/clob/types/expected_keepers.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Applied to files:

  • protocol/x/perpetuals/keeper/perpetual.go
📚 Learning: 2024-11-15T16:00:11.304Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:852-865
Timestamp: 2024-11-15T16:00:11.304Z
Learning: The function `GetCrossInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Applied to files:

  • protocol/x/perpetuals/keeper/perpetual.go
📚 Learning: 2024-11-15T15:59:28.095Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2551
File: protocol/x/subaccounts/keeper/subaccount.go:833-850
Timestamp: 2024-11-15T15:59:28.095Z
Learning: The function `GetInsuranceFundBalance` in `protocol/x/subaccounts/keeper/subaccount.go` already existed and was just moved in this PR; changes to its error handling may be out of scope.

Applied to files:

  • protocol/x/perpetuals/keeper/perpetual.go
📚 Learning: 2025-01-25T19:55:32.807Z
Learnt from: UnbornAztecKing
Repo: dydxprotocol/v4-chain PR: 2704
File: protocol/streaming/full_node_streaming_manager.go:205-218
Timestamp: 2025-01-25T19:55:32.807Z
Learning: In the streaming manager's subaccount filtering logic, `slices.Contains` is used instead of a map-based set as it's an optimization for small sets of target subaccountIds, where the overhead of creating and maintaining a hash map would outweigh the benefits.

Applied to files:

  • protocol/x/perpetuals/keeper/perpetual.go
  • protocol/x/clob/keeper/grpc_query_leverage.go
📚 Learning: 2025-10-06T20:00:48.549Z
Learnt from: anmolagrawal345
Repo: dydxprotocol/v4-chain PR: 3079
File: protocol/x/clob/types/leverage.go:10-56
Timestamp: 2025-10-06T20:00:48.549Z
Learning: In the leverage update flow for protocol/x/clob, the `ValidateUpdateLeverageMsg` function performs basic validation (non-nil checks, non-zero leverage, unique IDs, valid clob pair existence), while maximum leverage validation against `GetMaxLeverageForPerpetual` is intentionally deferred to the `UpdateLeverage` keeper method.

Applied to files:

  • protocol/x/clob/keeper/grpc_query_leverage.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (4)
protocol/x/prices/client/testutil/util.go (1)

4-4: LGTM! Clean refactor to slices.Sort.

The change from sort.Slice to slices.Sort correctly preserves the ascending order sorting behavior while improving code conciseness.

Also applies to: 15-15

protocol/x/clob/keeper/grpc_query_leverage.go (1)

5-5: LGTM! Deterministic ordering preserved.

The refactor to slices.Sort correctly maintains the deterministic ascending order for the leverage response keys while simplifying the code.

Also applies to: 40-40

protocol/x/perpetuals/keeper/perpetual.go (1)

7-8: LGTM! Both imports correctly retained.

The refactor to slices.Sort for the []int32 slice is appropriate. The sort package import is correctly retained for the custom struct sorting at lines 251-253 and 1403-1405.

Also applies to: 619-619

protocol/lib/math.go (1)

8-8: Verification confirmed: Go version requirement is met.

The project's go.mod specifies go 1.23.1, which exceeds the minimum Go 1.21 requirement for slices.Sort. The refactor is safe and appropriate.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ledigang
Copy link
Author

refactor label

@ledigang
Copy link
Author

@tqin7 Hi, Could you please review this PR at your convenience? Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

1 participant