Avoid same peer rebalances#522
Open
markettes wants to merge 6 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes rebalance requests stricter and prevents “no-op” rebalances by requiring SourceChannelId and TargetPubkey end-to-end (UI → gRPC → domain), and adding a guard to reject selecting the source channel’s counterparty as the receiving peer.
Changes:
- Make
RebalanceRequest.SourceChannelIdandRebalanceRequest.TargetPubkeymandatory and validate them early inRebalanceService.RebalanceAsync. - Add gRPC-layer
InvalidArgumentguards for missing/invalidsource_channel_idandtarget_pubkeybefore the node lookup. - Update the rebalance modal to require a receiving peer and filter out the source channel counterparty; expand/adjust test coverage accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/NodeGuard.Tests/Services/RebalanceServiceTests.cs | Updates existing tests for new required request fields and adds new validation/no-op-guard tests. |
| test/NodeGuard.Tests/Rpc/NodeGuardServiceTests.cs | Adds gRPC validation tests for missing required fields and updates existing cases to reach intended code paths. |
| src/Shared/NewRebalanceModal.razor | Makes receiving peer required, filters out counterparty peer, and enforces submission constraints in the UI. |
| src/Services/RebalanceService.cs | Changes RebalanceRequest to non-nullable fields and adds validation + no-op counterparty guard in RebalanceAsync. |
| src/Rpc/NodeGuardService.cs | Rejects missing/invalid source_channel_id and target_pubkey with InvalidArgument before calling the domain service. |
| src/Proto/nodeguard.proto | Updates field comments to mark source_channel_id and target_pubkey as required (keeping wire optional). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… and mandatory channel resolution
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
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.
clear ArgumentException.
both required (the optional wire-protocol modifier stays so existing serialized payloads still parse).
and accomplish nothing.
both fields and forbids the no-op combination; if the user changes the source channel and the previously-picked receiving peer becomes invalid, it gets cleared.
The Rebalance model fields are intentionally left nullable in this PR — tightening the DB schema needs a data backfill / cleanup decision that's out of scope here.