YNU-729: feat(ChannelHub): add cross-chain deposit, withdrawal, migration#498
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces cross-chain escrow and migration support for a payment channels protocol. It adds two new engine libraries for escrow deposit/withdrawal validation, extends ChannelsHub with escrow and migration functions, refactors delta calculations in ChannelEngine, updates type enums to support escrow and migration states, and includes comprehensive test coverage for cross-chain lifecycle scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NonHomeChain as Non-Home Chain<br/>(ChannelsHub)
participant Engine as EscrowDepositEngine
participant HomeChain as Home Chain<br/>(State Record)
User->>NonHomeChain: initiateEscrowDeposit(definition, candidate)
NonHomeChain->>Engine: validateTransition(ctx, candidate)
Engine->>Engine: Validate INITIATE_ESCROW_DEPOSIT<br/>(lock user funds, check states)
Engine-->>NonHomeChain: Return TransitionEffects
NonHomeChain->>NonHomeChain: Apply effects<br/>(status=INITIALIZED, escrow created)
Note over NonHomeChain: Unlock delay timer starts
rect rgba(100, 149, 237, 0.5)
Note over User,HomeChain: Time passes (unlock delay)
end
User->>NonHomeChain: finalizeEscrowDeposit(escrowId, candidate)
NonHomeChain->>Engine: validateTransition(ctx, candidate)
Engine->>Engine: Validate FINALIZE_ESCROW_DEPOSIT<br/>(check funds, validate home state)
Engine-->>NonHomeChain: Return TransitionEffects
NonHomeChain->>NonHomeChain: Apply effects<br/>(status=FINALIZED, release to node)
NonHomeChain-->>User: Escrow deposit finalized
sequenceDiagram
participant User
participant NonHomeChain as Non-Home Chain<br/>(ChannelsHub)
participant ChannelEngine
participant HomeChain as Home Chain<br/>(State Record)
User->>NonHomeChain: initiateMigration(definition, candidate)
NonHomeChain->>ChannelEngine: validateTransition(ctx, candidate)
ChannelEngine->>ChannelEngine: Validate INITIATE_MIGRATION<br/>(check channel exists, state consistency)
ChannelEngine-->>NonHomeChain: Return TransitionEffects
NonHomeChain->>NonHomeChain: Apply effects<br/>(status=MIGRATING_IN)
rect rgba(144, 238, 144, 0.5)
Note over User,HomeChain: Migration preparation phase
end
User->>NonHomeChain: finalizeMigration(channelId, candidate)
NonHomeChain->>ChannelEngine: validateTransition(ctx, candidate)
ChannelEngine->>ChannelEngine: Validate FINALIZE_MIGRATION<br/>(swap home/non-home states)
ChannelEngine-->>NonHomeChain: Return TransitionEffects
NonHomeChain->>NonHomeChain: Apply effects<br/>(status=MIGRATED_OUT, state swap complete)
NonHomeChain-->>User: Migration finalized
Note over NonHomeChain: Non-home becomes home<br/>Previous home state becomes non-home
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Summary of ChangesHello @nksazonov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the ChannelsHub protocol by integrating comprehensive support for cross-chain operations. It introduces robust mechanisms for users to securely deposit and withdraw funds across different blockchain networks through a new escrow system. The changes involve substantial modifications to the core Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for cross-chain deposits, withdrawals, and migrations within the state channel system. It adds new EscrowDepositEngine and EscrowWithdrawalEngine contracts to manage cross-chain operations, and extensively modifies ChannelEngine and ChannelsHub to integrate these new state transitions. The changes are substantial, including new storage structures, state machine intents, and event logging. While the overall architecture is well-thought-out and the inclusion of new tests is commendable, my review has identified a few critical issues. These include a potential denial-of-service vector due to an unbounded loop, and incorrect logic in state transition validation that could block functionality. I have also noted some areas for improvement in code efficiency and clarity.
| // state from the "happyPath" test, but with home and nonHome states swapped | ||
| CrossChainState memory state = CrossChainState({ | ||
| version: 42, | ||
| intent: StateIntent.INITIATE_ESCROW_DEPOSIT, |
There was a problem hiding this comment.
It is not very clear why do we provide StateIntent.INITIATE_ESCROW_DEPOSIT for a test for migration, and how does the contract understands that we want to execute migration
There was a problem hiding this comment.
Yes, your are correct. It was a bug, which is now fixed.
| userAllocation: 0, | ||
| userNetFlow: 0, | ||
| nodeAllocation: 469, | ||
| nodeNetFlow: 469 |
There was a problem hiding this comment.
I suggest using simpler numbers to calculate instead of 469 and 281 as these numbers make it difficult to review when you are trying to calcalate it in your head to verify
There was a problem hiding this comment.
It gets much more difficult to match all amounts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts-new/src/ChannelEngine.sol (1)
501-519: Use SafeCast forctx.lockedFundsto avoid silent truncation.
int256(ctx.lockedFunds)can wrap iflockedFundsexceedsint256.max, breaking invariant checks. SafeCast is already imported and used consistently throughout the file for this purpose.🛠️ Suggested fix
- int256 expectedLocked = - int256(ctx.lockedFunds) + effects.userFundsDelta + effects.nodeFundsDelta; + int256 expectedLocked = + ctx.lockedFunds.toInt256() + effects.userFundsDelta + effects.nodeFundsDelta;
🤖 Fix all issues with AI agents
In `@contracts-new/protocol-description.md`:
- Line 487: The fenced code block that contains the expression "delta =
candidate.homeState.netFlow - prevStoredState.homeState.netFlow" is missing a
language identifier (triggering markdownlint MD040); update that specific fenced
block in protocol-description.md to include a language tag (e.g., "text" or
"plain") immediately after the opening backticks so the block becomes a labeled
fenced code block and the linter warning is resolved.
- Around line 460-466: The docs list four functions
(initiateMigrationIn/initiateMigrationOut/finalizeMigrationIn/finalizeMigrationOut)
that don't exist on ChannelsHub; either update the protocol doc to reference the
actual ChannelsHub API (initiateMigration and finalizeMigration) and clarify how
direction/home-chain behavior is encoded, or add wrapper methods on ChannelsHub
named
initiateMigrationIn/initiateMigrationOut/finalizeMigrationIn/finalizeMigrationOut
that internally call the existing initiateMigration/finalizeMigration methods
(or forward the appropriate parameters) and update all references; ensure you
update the docs/tests to reference the chosen approach and keep function names
consistent (ChannelsHub.initiateMigration, ChannelsHub.finalizeMigration or the
new wrappers) so readers and code match.
In `@contracts-new/src/ChannelsHub.sol`:
- Around line 773-779: _isHomeChain currently only treats ChannelStatus.VOID as
non-home, causing channels with status ChannelStatus.MIGRATED_OUT to be
considered home; update _isHomeChain to also return false when
_channels[channelId].status == ChannelStatus.MIGRATED_OUT so migrated-out
channels are treated as non-home (i.e., add a status check for MIGRATED_OUT
alongside VOID before comparing lastState.homeState.chainId to block.chainid).
- Around line 102-106: getAccountsBalances currently returns an empty matrix;
implement it to allocate a uint256[][] memory result with length
accounts.length, then for each account allocate an inner
uint256[](tokens.length) and populate result[i][j] by calling
IERC20(tokens[j]).balanceOf(accounts[i]) (handle token==address(0) as native
balance using accounts[i].balance if your contract expects native balances). Use
the IERC20 interface (balanceOf) in this contract or import it, keep the
function as external view and return the filled 2D array from
getAccountsBalances.
- Around line 277-299: _purgeEscrowDeposits currently breaks when encountering
any non-purgeable entry, which deadlocks if an entry is already
EscrowStatus.FINALIZED; instead advance escrowHead past FINALIZED entries.
Modify _purgeEscrowDeposits to: when iterating _escrowDepositIds use
escrowHeadTemp, if meta.status == EscrowStatus.FINALIZED then just
escrowHeadTemp++ and continue (do not attempt to re-credit funds or count
towards purgedCount); if meta.status == EscrowStatus.CHALLENGED or meta.unlockAt
> block.timestamp then break as before; otherwise handle INITIALIZED expirations
as existing logic and increment escrowHeadTemp. Ensure escrowHead is updated to
escrowHeadTemp at the end.
In `@contracts-new/src/EscrowDepositEngine.sol`:
- Around line 143-175: In _calculateFinalizeEffects replace the unsafe uint→int
casts with SafeCast conversions: use depositAmount.toInt256() (negated as
-depositAmount.toInt256() where needed) instead of -int256(depositAmount) in the
candidate.nonHomeState.nodeNetFlow check, and use -ctx.lockedAmount.toInt256()
instead of -int256(ctx.lockedAmount) when setting effects.nodeFundsDelta; this
keeps the function (and symbols depositAmount, ctx.lockedAmount,
candidate.nonHomeState.nodeNetFlow, effects.nodeFundsDelta) safe from uint→int
overflow using the existing SafeCast utilities.
♻️ Duplicate comments (4)
contracts-new/src/ChannelEngine.sol (2)
322-353: Assign node delta tonodeFundsDelta.Line 348 assigns
nodeNfDeltatouserFundsDelta, but the comment and intent indicate only node balance adjustments. This misroutes funds and breaks invariants.🐛 Suggested fix
- effects.userFundsDelta = nodeNfDelta; // Only node balance adjustments + effects.nodeFundsDelta = nodeNfDelta; // Only node balance adjustments
355-388: Finalize withdrawal non-home invariants look inconsistent.The current checks require
userNetFlow == 0anduserAllocation == nodeNetFlow, which fails for a valid finalized withdrawal whereuserAllocation == 0andnodeNetFlowis positive (withuserNetFlownegative). This blocks finalization.🐛 Suggested fix
- require(candidate.nonHomeState.userNetFlow == 0, "withdrawal user net flow must be zero"); - require(candidate.nonHomeState.nodeAllocation == 0, "withdrawal node allocation must be zero"); - require( - candidate.nonHomeState.userAllocation.toInt256() == candidate.nonHomeState.nodeNetFlow, - "invalid non-home user net flow" - ); + require(candidate.nonHomeState.userAllocation == 0, "withdrawal user allocation must be zero"); + require(candidate.nonHomeState.nodeAllocation == 0, "withdrawal node allocation must be zero"); + require( + candidate.nonHomeState.userNetFlow == -candidate.nonHomeState.nodeNetFlow, + "invalid non-home net flows" + );contracts-new/src/ChannelsHub.sol (2)
118-135: Avoid the temporary array copy ingetOpenChannels.
This allocates a full-length temp array then copies into a smaller one; a count-first pass avoids the extra allocation.♻️ Suggested refactor
- bytes32[] memory openChannelsTemp = new bytes32[](allChannels.length); - uint256 count = 0; - for (uint256 i = 0; i < allChannels.length; i++) { - bytes32 channelId = allChannels[i]; - ChannelMeta memory meta = _channels[channelId]; - if (meta.status != ChannelStatus.CLOSED && meta.status != ChannelStatus.MIGRATED_OUT) { - openChannelsTemp[count] = channelId; - count++; - } - } - bytes32[] memory openChannels = new bytes32[](count); - for (uint256 i = 0; i < count; i++) { - openChannels[i] = openChannelsTemp[i]; - } - return openChannels; + uint256 count = 0; + for (uint256 i = 0; i < allChannels.length; i++) { + ChannelMeta memory meta = _channels[allChannels[i]]; + if (meta.status != ChannelStatus.CLOSED && meta.status != ChannelStatus.MIGRATED_OUT) { + count++; + } + } + bytes32[] memory openChannels = new bytes32[](count); + uint256 idx = 0; + for (uint256 i = 0; i < allChannels.length; i++) { + bytes32 channelId = allChannels[i]; + ChannelMeta memory meta = _channels[channelId]; + if (meta.status != ChannelStatus.CLOSED && meta.status != ChannelStatus.MIGRATED_OUT) { + openChannels[idx++] = channelId; + } + } + return openChannels;
78-84: Cap automatic escrow purges to avoid OOG/DoS.
Line 83 setsMAX_DEPOSIT_ESCROW_PURGEtotype(uint32).max. Because_purgeEscrowDeposits()runs inside state transitions, a large backlog can make core operations revert due to gas exhaustion.🔧 Suggested change
- uint32 public constant MAX_DEPOSIT_ESCROW_PURGE = type(uint32).max; + uint32 public constant MAX_DEPOSIT_ESCROW_PURGE = 20;
|
@coderabbitai review |
dimast-x
changed the title
| require(candidate.intent == StateIntent.INITIATE_MIGRATION, "invalid intent"); | ||
|
|
||
| bytes32 channelId = Utils.getChannelId(def); | ||
| candidate.validateNodeSignature(channelId, def.node); |
There was a problem hiding this comment.
It seems it is safe to not require user signature for one less user interaction.
This is safe, because no user funds are moved during this step - only Node deposits homeChainUserAmount on non-home chain.
| event EscrowDepositInitiatedOnHome(bytes32 indexed escrowId, bytes32 indexed channelId, State state); | ||
| event EscrowDepositChallenged(bytes32 indexed escrowId, State state, uint64 challengeExpireAt); | ||
| event EscrowDepositFinalized(bytes32 indexed escrowId, State state); | ||
| event EscrowDepositFinalizedOnHome(bytes32 indexed escrowId, State state); |
There was a problem hiding this comment.
We also need channel ID to be returned in:
- EscrowDepositFinalizedOnHome
- EscrowWithdrawalFinalizedOnHome
alessio
left a comment
alessio
left a comment
There was a problem hiding this comment.
Looks good to me. Keep up the great work!
4 participants
Summary by CodeRabbit
New Features
Optimization
✏️ Tip: You can customize this high-level summary in your review settings.