feat(v11): add elys xrp withdrawal#147
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:
📝 WalkthroughWalkthroughThe v11 upgrade handler is extended to drain stranded funds from the Elys IBC transfer channel escrow account. New constants for mainnet, testnet, and devnet ( Changesv11 Elys escrow withdrawal
POA test mock fix
Sequence Diagram(s)sequenceDiagram
participant UpgradeHandler
participant withdrawElysEscrow
participant TransferKeeper
UpgradeHandler->>withdrawElysEscrow: call(ctx, logger, bankKeeper, transferKeeper)
withdrawElysEscrow->>withdrawElysEscrow: lookup ElysRecoveryByNetwork[chainID]
alt no recovery config for chainID
withdrawElysEscrow-->>UpgradeHandler: return nil (skip)
else recovery configured and amount positive
withdrawElysEscrow->>withdrawElysEscrow: GetEscrowAddress(channelID)
withdrawElysEscrow->>withdrawElysEscrow: bech32.DecodeAndConvert(WithdrawalAddress)
withdrawElysEscrow->>TransferKeeper: UnescrowCoin(escrowAddr, withdrawalAddr, coin)
withdrawElysEscrow-->>UpgradeHandler: log result, return nil/error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app/upgrades/v11/constants.go`:
- Around line 10-12: The WithdrawalAddress constant in constants.go has a TODO
comment indicating it is not finalized, but this address is actively used in
upgrades.go to transfer the full escrow balance during upgrade execution. This
creates a critical risk of permanently sending funds to the wrong account.
Verify and finalize the correct withdrawal address value, replace the current
address with the verified correct address, and remove the TODO comment to
indicate that the value has been confirmed and is ready for production use.
In `@app/upgrades/v11/upgrades.go`:
- Around line 73-74: The subtraction operation in
transferKeeper.SetTotalEscrowForDenom(ctx, totalEscrow.Sub(escrowBalance)) lacks
validation and will panic if escrowBalance exceeds totalEscrow due to
inconsistent transfer accounting. Add a bounds check before line 74 to validate
that escrowBalance is less than or equal to totalEscrow retrieved on line 73. If
the check fails, log an appropriate error message and return early from the
function rather than proceeding with the subtraction that would cause a panic
during the upgrade.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbf78936-c778-48f0-a14d-62eed2699428
📒 Files selected for processing (4)
app/upgrades.goapp/upgrades/v11/constants.goapp/upgrades/v11/keepers.goapp/upgrades/v11/upgrades.go
| ElysChannelID = "channel-1" | ||
|
|
||
| // ElysEscrowAddress is the Elys channel's escrow account, pinned as a guard. | ||
| ElysEscrowAddress = "ethm1kq2rzz6fq2q7fsu75a9g7cpzjeanmk68nttack" |
There was a problem hiding this comment.
Set different address for devnet, testnet, and mainnet.
There was a problem hiding this comment.
No need to store ElysEscrowAddress if we have ElysChannelID
| const ( | ||
| UpgradeName = "v11.0.0" | ||
| UpgradeName = "v11.0.0" | ||
| ElysChannelID = "channel-1" |
There was a problem hiding this comment.
Set different channel id for devnet, testnet and mainnet
| // escrow to the recovery address and updates the total-escrow accounting. | ||
| func withdrawElysEscrow(ctx sdk.Context, logger log.Logger, bankKeeper BankKeeper, transferKeeper TransferKeeper) error { | ||
| elysEscrowAddr := transfertypes.GetEscrowAddress(transfertypes.PortID, ElysChannelID) | ||
| if elysEscrowAddr.String() != ElysEscrowAddress { |
There was a problem hiding this comment.
This validation is not needed
|
|
||
| // XRP base denom from the canonical EVM coin config. | ||
| xrpDenom := evmtypes.GetEVMCoinDenom() | ||
| elysEscrowBalance := bankKeeper.GetBalance(ctx, elysEscrowAddr, xrpDenom) |
There was a problem hiding this comment.
Use GetBalances instead of only withdrawing xrp
| return fmt.Errorf("failed to withdraw elys escrow: %w", err) | ||
| } | ||
|
|
||
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, xrpDenom) |
There was a problem hiding this comment.
Attach a report explaning why we need to SetTotalEscrow
There was a problem hiding this comment.
The transfer module keeps a per-denom counter TotalEscrowForDenom that must equal the real balance in its escrow accounts. EscrowCoin and UnescrowCoin always move the bank balance and this counter together.
We withdraw the Elys channel with a raw SendCoins, which doesn't touch the counter. So we decrement it ourselves, or it permanently over-reports withdrawn escrow, breaking the invariant and corrupting exported genesis (GetAllTotalEscrowed). We subtract only the withdrawn amount (the same denom may be escrowed on other live channels), which also keeps it non-negative so SetTotalEscrowForDenom won't panic.
There was a problem hiding this comment.
Use UnescrowCoin function then
…bonding lookup PR xrplevm#126 (fix/poa-unbonding-delegation-check) switched the AddValidator eligibility check to k.sk.GetUnbondingDelegations(ctx, accAddress, ...) and updated the interface, mock and keeper_test.go, but missed this msg_server test, which still mocked GetUnbondingDelegationsFromValidator. The stale expectation makes the real call unexpected, failing make test-poa and make coverage-unit on the PR-merge build. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve v11 upgrade conflicts by unioning both features: main's approved staking unbonding-period change (1 week) plus this branch's elys escrow withdrawal. The local WIP unbonding duplicate (30d) was dropped in favor of main's version (stashed, not committed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Take origin's elys implementation (UnescrowCoin / IterateTokensInEscrow, per-network recovery) and main's 1-week unbonding. The only delta over origin is the POA add-validator test mock fix (GetUnbondingDelegations), which origin was still missing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 `@app/upgrades/v11/upgrades.go`:
- Around line 78-93: The code is calling UnescrowCoin within the
IterateTokensInEscrow callback, which violates the Cosmos SDK contract that
prohibits writes to the same state domain while an iterator is open over it.
This can cause panics or halt the chain. Instead, collect the coins to be
unescrow during the iteration by storing them in a slice or similar data
structure within the callback (returning false to continue iteration without
calling UnescrowCoin), then after the IterateTokensInEscrow iterator closes,
iterate over the collected coins and call UnescrowCoin for each one to safely
perform the escrow state modifications.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8eab54a8-12ec-4577-9ec6-e27c1888062c
📒 Files selected for processing (4)
app/upgrades.goapp/upgrades/v11/keepers.goapp/upgrades/v11/upgrades.gox/poa/keeper/msg_server_add_validator_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- app/upgrades.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app/upgrades/v11/constants.go`:
- Around line 16-21: TestnetWithdrawalAddress and DevnetWithdrawalAddress
constants are currently set to the same value, which may indicate a copy-paste
error given that different networks should typically have distinct withdrawal
addresses for fund routing purposes. Review the intended configuration for the
devnet environment and either update DevnetWithdrawalAddress with the correct
distinct address for the devnet network, or document and confirm with the team
that sharing the same withdrawal address between testnet and devnet is
intentional.
In `@app/upgrades/v11/upgrades.go`:
- Around line 62-85: The function `withdrawElysEscrow` passes `bankKeeper` as a
parameter but never uses it, and lacks validation to safely cap the withdrawal
amount before calling `transferKeeper.UnescrowCoin`. Either remove the unused
`bankKeeper` parameter if the hardcoded `recoveryCfg.Amount` is confirmed to
exactly match the on-chain Elys channel escrow balance, or introduce a safety
cap by using `bankKeeper.GetBalance` to retrieve the actual escrow balance and
apply the minimum of that value and `recoveryCfg.Amount` before the
`UnescrowCoin` call to prevent errors or panics. Additionally, verify that
`evmtypes.GetEVMCoinDenom()` returns the exact denomination held in the Elys
channel escrow account, and confirm the hardcoded mainnet amount matches the
actual stranded balance at the upgrade height.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68ba1876-5f9d-4555-8356-544029fae7c2
📒 Files selected for processing (3)
app/upgrades/v11/constants.goapp/upgrades/v11/keepers.goapp/upgrades/v11/upgrades.go
💤 Files with no reviewable changes (1)
- app/upgrades/v11/keepers.go
| TestnetWithdrawalAddress = "ethm1p95fctckyrxuxu6t47e2uuckjl9tfuxynuawsc" | ||
|
|
||
| // devnet | ||
| DevnetChainID = "xrplevm_1449900-1" | ||
| DevnetElysChannelID = "channel-4" | ||
| DevnetWithdrawalAddress = "ethm1p95fctckyrxuxu6t47e2uuckjl9tfuxynuawsc" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Testnet and devnet share the same WithdrawalAddress.
TestnetWithdrawalAddress (Line 16) and DevnetWithdrawalAddress (Line 21) are identical (ethm1p95fctckyrxuxu6t47e2uuckjl9tfuxynuawsc), while a previous review asked for distinct per-network addresses. Please confirm this reuse is intentional and not a copy-paste leftover, since funds are routed irreversibly to this address during the upgrade.
🤖 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 `@app/upgrades/v11/constants.go` around lines 16 - 21, TestnetWithdrawalAddress
and DevnetWithdrawalAddress constants are currently set to the same value, which
may indicate a copy-paste error given that different networks should typically
have distinct withdrawal addresses for fund routing purposes. Review the
intended configuration for the devnet environment and either update
DevnetWithdrawalAddress with the correct distinct address for the devnet
network, or document and confirm with the team that sharing the same withdrawal
address between testnet and devnet is intentional.
| func withdrawElysEscrow(ctx sdk.Context, logger log.Logger, bankKeeper BankKeeper, transferKeeper TransferKeeper) error { | ||
| recoveryCfg, ok := ElysRecoveryByNetwork[ctx.ChainID()] | ||
| if !ok || recoveryCfg.ChannelID == "" { | ||
| logger.Info("no Elys escrow recovery configured for this network, skipping", "chainID", ctx.ChainID()) | ||
| return nil | ||
| } | ||
|
|
||
| if !recoveryCfg.Amount.IsPositive() { | ||
| logger.Info("Elys escrow recovery amount is zero, nothing to withdraw", "chainID", ctx.ChainID()) | ||
| return nil | ||
| } | ||
|
|
||
| escrowAddr := transfertypes.GetEscrowAddress(transfertypes.PortID, recoveryCfg.ChannelID) | ||
|
|
||
| destAddr, err := sdk.AccAddressFromBech32(recoveryCfg.WithdrawalAddress) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid withdrawal address %q: %w", recoveryCfg.WithdrawalAddress, err) | ||
| } | ||
|
|
||
| coin := sdk.NewCoin(evmtypes.GetEVMCoinDenom(), recoveryCfg.Amount) | ||
|
|
||
| if err := transferKeeper.UnescrowCoin(ctx, escrowAddr, destAddr, coin); err != nil { | ||
| return fmt.Errorf("failed to unescrow %s from elys escrow: %w", coin, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In cosmos/ibc-go v10.4.0, does TransferKeeper.UnescrowCoin return an error or panic when the coin amount exceeds the escrow account balance or the TotalEscrowForDenom counter?
💡 Result:
In ibc-go v10.4.0, the TransferKeeper.UnescrowCoin function does not explicitly check if the coin amount exceeds the escrow account balance or the TotalEscrowForDenom counter; instead, it relies on the underlying BankKeeper and Cosmos SDK arithmetic to handle these operations, which leads to different outcomes [1][2]. 1. Escrow Account Balance: The function calls k.BankKeeper.SendCoins to move tokens from the escrow address to the receiver [1][2]. If the escrow account has insufficient balance to cover the requested amount, the BankKeeper.SendCoins method will return an error [1]. In this case, UnescrowCoin wraps and returns that error, rather than panicking [1][2]. 2. TotalEscrowForDenom Counter: After sending the coins, the function calls currentTotalEscrow.Sub(coin) [1][2]. If the coin amount exceeds the current total escrow stored in the state, the Cosmos SDK's Int.Sub method (or similar Sub operation on the Coin type) will typically panic if it results in a negative amount [3]. While the code tracks escrow, it assumes the operation is valid based on the successful bank transfer; if the escrow state becomes inconsistent (e.g., if more tokens are unescrowed than were ever escrowed due to a malicious counterparty), this subtraction operation will trigger a panic [1][2]. The documentation explicitly notes that if a negative amount is ever attempted to be stored, the keeper function will panic [3]. Therefore, while the function handles bank-level insufficient funds via error propagation, an inconsistency between the actual escrowed tokens and the tracked TotalEscrowForDenom state will lead to a panic during the subtraction step [1][3].
Citations:
- 1: https://github.com/cosmos/ibc-go/blob/main/modules/apps/transfer/keeper/relay.go
- 2: https://github.com/cosmos/ibc-go/blob/25010f7b/modules/apps/transfer/keeper/relay.go
- 3: https://github.com/cosmos/ibc-go/blob/25010f7b/docs/architecture/adr-011-transfer-total-escrow-state-entry.md
🏁 Script executed:
# First, let's look at the full function and surrounding context
cat -n app/upgrades/v11/upgrades.go | head -100Repository: xrplevm/node
Length of output: 3734
🏁 Script executed:
# Search for ElysRecoveryByNetwork definition and bankKeeper usage
rg "ElysRecoveryByNetwork|BankKeeper" app/upgrades/v11/ -A 3 -B 1Repository: xrplevm/node
Length of output: 2047
🏁 Script executed:
# Search for GetEVMCoinDenom implementation
rg "GetEVMCoinDenom" --type go -A 5Repository: xrplevm/node
Length of output: 150
🏁 Script executed:
# Search for GetEVMCoinDenom - it's imported from evmtypes
rg "func.*GetEVMCoinDenom" --type go -A 5Repository: xrplevm/node
Length of output: 150
🏁 Script executed:
# Look at the constants file to see ElysRecoveryByNetwork and related values
cat -n app/upgrades/v11/constants.goRepository: xrplevm/node
Length of output: 2372
🏁 Script executed:
# Also check if there are any other usages of bankKeeper in the upgrade file
rg "bankKeeper\." app/upgrades/v11/Repository: xrplevm/node
Length of output: 150
🌐 Web query:
cosmos/evm GetEVMCoinDenom what denom does it return
💡 Result:
In the Cosmos EVM module, the function GetEVMCoinDenom() returns the base denomination (a string) used as the native EVM token for the chain [1][2]. This value represents the specific token denomination from the bank module that is used for EVM state transitions, gas consumption, and other native EVM operations [3][4]. By default, this is typically set to "uatom" [5][3], but it is a configurable parameter that must match the chain's native staking and minting denominations [3][6]. It is important to distinguish this from other denominations used in the module: - GetEVMCoinDenom: Returns the base denomination (e.g., "uatom") [1][2]. - GetEVMCoinExtendedDenom: Returns the extended denomination used for 18-decimal representation if configured [2]. - GetEVMCoinDisplayDenom: Returns the human-readable display denomination (e.g., "atom") [2].
Citations:
- 1: https://context7.com/cosmos/evm/llms.txt
- 2: cosmos/evm@b72eb13
- 3: https://cosmos-docs.mintlify.app/evm/v0.5.0/documentation/cosmos-sdk/modules/vm
- 4: https://docs.ethermint.zone/modules/evm/07_params
- 5: https://github.com/cosmos/evm/blob/main/x/vm/types/params.go
- 6: https://cosmos-docs.mintlify.app/evm/next/documentation/cosmos-sdk/modules/vm
🏁 Script executed:
# Check if there are any comments or documentation about the denom in this file
rg "denom|Denom|DENOM" app/upgrades/v11/Repository: xrplevm/node
Length of output: 353
Remove unused bankKeeper parameter or apply balance safety cap before UnescrowCoin.
The function passes bankKeeper but never invokes GetBalance to cap the withdrawal. With a hardcoded recoveryCfg.Amount, the code relies on that value exactly matching the on-chain stranded balance:
- If
recoveryCfg.Amountexceeds the escrow account's balance for the denom,UnescrowCoinreturns an error and the upgrade halts. - If
recoveryCfg.Amountexceeds the trackedTotalEscrowForDenom, the subtraction panics during the upgrade. - If
recoveryCfg.Amountis less than the actual stranded balance, funds remain stranded.
For mainnet, mainnetElysAmount is hardcoded to "6955539034646993768414" with a comment claiming it is the "exact XRP stranded." Confirm this value precisely matches the on-chain Elys channel escrow balance at upgrade height. If the hardcoded amount is intentional, remove the now-dead bankKeeper parameter and its interface wiring. Otherwise, re-introduce bankKeeper.GetBalance to safely cap the amount.
Also verify that evmtypes.GetEVMCoinDenom() returns the exact denomination held in the Elys channel escrow account, otherwise UnescrowCoin will operate on the wrong denom.
🤖 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 `@app/upgrades/v11/upgrades.go` around lines 62 - 85, The function
`withdrawElysEscrow` passes `bankKeeper` as a parameter but never uses it, and
lacks validation to safely cap the withdrawal amount before calling
`transferKeeper.UnescrowCoin`. Either remove the unused `bankKeeper` parameter
if the hardcoded `recoveryCfg.Amount` is confirmed to exactly match the on-chain
Elys channel escrow balance, or introduce a safety cap by using
`bankKeeper.GetBalance` to retrieve the actual escrow balance and apply the
minimum of that value and `recoveryCfg.Amount` before the `UnescrowCoin` call to
prevent errors or panics. Additionally, verify that `evmtypes.GetEVMCoinDenom()`
returns the exact denomination held in the Elys channel escrow account, and
confirm the hardcoded mainnet amount matches the actual stranded balance at the
upgrade height.
2 participants
Add xrp withdrawal from elys channel as part of the v11 handler
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests