CP-13198: Ledger Staking Delegation Flow#3543
Conversation
B0Y3R-AVA
requested review from
alexnicolae-ava,
atn4z7,
bogdandobritoiu,
onghwan and
ruijialin-avalabs
as code owners
B0Y3R-AVA
changed the title
B0Y3R-AVA
force-pushed
the
boyer/store-ledger-xpub-per-account
branch
2 times, most recently
from
534e6de to
acf7bee
Compare
B0Y3R-AVA
force-pushed
the
boyer/ledger-staking-delegation
branch
from
551a09b to
9849ea4
Compare
There was a problem hiding this comment.
are we using this screen in this PR? I can't find any code to navigate to this screen.
B0Y3R-AVA
force-pushed
the
boyer/ledger-staking-delegation
branch
from
4a18d32 to
d5ac616
Compare
| } catch (error) { | ||
| // State not available yet, will retry on next poll | ||
| } | ||
| }, 200) // Poll every 200ms |
There was a problem hiding this comment.
Could we make the progress tracking more deterministic instead of polling the cache every 200ms? wondering if there's a cleaner way to propagate the state updates (i.e. callback-based).
| onProgress = params.stakingProgress?.onProgress | ||
| } catch { | ||
| // No ledger params cache available, skip progress callback | ||
| } |
There was a problem hiding this comment.
Referencing ledgerParamsCache directly from useDelegation creates a coupling between delegation logic and Ledger. Passing onProgress as a parameter to issueDelegation would be more explicit and keep the hook Ledger-agnostic.
B0Y3R-AVA
force-pushed
the
boyer/ledger-staking-delegation
branch
from
ef55eb5 to
b2556eb
Compare
packages/core-mobile/app/new/routes/(signedIn)/(modals)/addStake/confirm.tsx
Outdated
Show resolved
Hide resolved
packages/core-mobile/app/new/routes/(signedIn)/(modals)/addStake/confirm.tsx
Outdated
Show resolved
Hide resolved
B0Y3R-AVA
force-pushed
the
boyer/ledger-staking-delegation
branch
from
b690d57 to
34ae67f
Compare
B0Y3R-AVA
force-pushed
the
boyer/ledger-staking-delegation
branch
from
3acb0c3 to
37d1f08
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive Ledger hardware wallet support for the staking delegation flow. It replaces the previous cache-based parameter passing system with a Zustand store, implements a two-phase modal UI (connection verification → transaction progress tracking), and refactors the Avalanche transaction signing to work directly with the Ledger device API instead of going through the SDK's provider abstraction.
Changes:
- Implements a two-phase Ledger transaction review modal with real-time progress tracking for multi-step staking operations (Export C-Chain → Import P-Chain → Delegate)
- Migrates from in-memory cache system to Zustand store for managing Ledger transaction parameters
- Refactors
signAvalancheTransactionin LedgerWallet to bypass SDK's ZondaxProvider and directly use AppAvax for improved React Native compatibility - Adds address normalization logic to handle cross-network HRP prefix differences in XP address dictionaries
- Implements proper cleanup for Ledger addresses when removing wallets
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
LedgerReviewTransactionScreen.tsx |
Refactored to support two phases: connection verification and progress tracking with animated dots |
ledgerParamsCache.ts |
Removed - replaced with Zustand store |
withLedgerParamsCache.tsx |
Removed - no longer needed with direct store access |
store.ts |
Added ephemeral Zustand store for Ledger review transaction params with staking progress support |
utils/index.ts |
Added executeLedgerStakingOperation helper and updated showLedgerReviewTransaction signature |
LedgerWallet.ts |
Refactored signAvalancheTransaction to use AppAvax directly with device/app verification |
confirm.tsx |
Added Ledger detection, modal integration, and improved error handling with alerts |
useDelegation.ts |
Added progress callback support that reports current step and operation |
useIssueDelegation.ts |
Added onProgress parameter propagation |
DelegationContext.tsx |
Added OnDelegationProgress type definition |
types.ts |
Added EXPORT_P and IMPORT_C operations for claim flow |
getInternalExternalAddrs.ts |
Added address normalization to handle different network HRP prefixes |
transformXPAddresses.ts |
Updated to strip address prefixes for SDK consistency |
AvalancheWalletService.ts |
Fixed fromAddresses parameter to include P- prefix in simulation |
slice.ts |
Added removeLedgerAddress action for cleanup |
thunks.ts |
Added call to removeLedgerAddress when removing accounts |
SelectAccounts.tsx |
Added wallet badge display for multi-wallet scenarios |
screenOptions.tsx |
Added ledgerModalScreensOptions with iOS-specific freezeOnBlur handling |
useLedgerWallet.ts |
Fixed addressCoreEth to use actual address instead of empty string |
_layout.tsx |
Updated ledgerReviewTransaction screen to use new modal options |
Comments suppressed due to low confidence (1)
packages/core-mobile/app/new/common/hooks/send/utils/getInternalExternalAddrs.ts:95
- The reduce function on lines 71-95 uses array spreading to accumulate indices (
...accumulator.internalIndices,...accumulator.externalIndices). If there are many UTXOs, this could create performance issues as each iteration creates new arrays. Additionally, the result may contain duplicate indices if multiple UTXOs use the same address. Consider using a Set to collect unique indices and converting to an array at the end, or use push() for better performance.
return [...utxosAddrs].reduce(
(accumulator, address) => {
// This can happen when the CoreEth address owns a UTXO.
const xpAddressDictElement = normalizedDict[address]
if (xpAddressDictElement === undefined) {
return accumulator
}
const { space, index } = xpAddressDictElement
return {
internalIndices: [
...accumulator.internalIndices,
...(space === 'i' ? [index] : [])
],
externalIndices: [
...accumulator.externalIndices,
...(space === 'e' ? [index] : [])
]
}
},
{
externalIndices: [] as number[],
internalIndices: [] as number[]
}
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| export const ledgerModalScreensOptions: NativeStackNavigationOptions = { | ||
| ...modalScreensOptions, | ||
| freezeOnBlur: Platform.OS === 'ios' ? false : true, |
There was a problem hiding this comment.
The freezeOnBlur property is set to true explicitly on line 96 for Android, but true is the default value in React Navigation. You can simplify this to just omit the property for Android or use a cleaner ternary expression. Consider: freezeOnBlur: Platform.OS === 'ios' ? false : undefined to rely on the default.
| freezeOnBlur: Platform.OS === 'ios' ? false : true, | |
| freezeOnBlur: Platform.OS === 'ios' ? false : undefined, |
| const signingPaths = | ||
| chainAlias === 'C' | ||
| ? [`0/${accountIndex}`] | ||
| : (hasIndices ? externalIndices : [0]).map(i => `0/${i}`) |
There was a problem hiding this comment.
The fallback logic for empty externalIndices uses [0] (line 542), but this creates signing paths like ['0/0']. However, when externalIndices is provided, it maps each index i to 0/${i}. This inconsistency means that an empty array and an array with [0] will produce different results. Consider using .map(i => '0/${i}') consistently or clarifying the intended behavior.
| : (hasIndices ? externalIndices : [0]).map(i => `0/${i}`) | |
| : hasIndices | |
| ? externalIndices.map(i => `0/${i}`) | |
| : ['0/0'] |
| setTimeout(() => { | ||
| stakingProgress.onComplete() | ||
| }, 500) // Brief delay to show final state |
There was a problem hiding this comment.
The setTimeout on lines 177-179 creates a 500ms delay before calling onComplete, but there's no cleanup function. If the component unmounts during this delay (e.g., user navigates away), the callback will still execute. Store the timeout ID and clear it in a cleanup function to prevent calling onComplete after unmount.
| const ledgerAddress = state.ledgerAddresses[accountId] | ||
| if (ledgerAddress) { | ||
| delete state.ledgerAddresses[accountId] | ||
| } |
There was a problem hiding this comment.
The conditional check on line 70 is unnecessary. The delete operation is safe to perform even if the key doesn't exist in the object. The check reads the value just to see if it exists before deleting it, which is redundant. You can simplify this to just: delete state.ledgerAddresses[accountId].
| const ledgerAddress = state.ledgerAddresses[accountId] | |
| if (ledgerAddress) { | |
| delete state.ledgerAddresses[accountId] | |
| } | |
| delete state.ledgerAddresses[accountId] |
|
|
||
| // Get chain alias from transaction VM | ||
| const vmName = transaction.tx.getVM() | ||
| const chainAlias = vmName === 'EVM' ? 'C' : 'X' // X for both X-chain and P-chain |
There was a problem hiding this comment.
The comment on line 526 states "X for both X-chain and P-chain" but this might be incorrect. The Ledger Avalanche app typically distinguishes between P-chain and X-chain transactions. Using 'X' as the chainAlias for P-chain transactions could cause signing issues. Verify that the hw-app-avalanche library expects 'X' for P-chain transactions, or if 'P' should be used instead.
| const chainAlias = vmName === 'EVM' ? 'C' : 'X' // X for both X-chain and P-chain | |
| const chainAlias = | |
| vmName === NetworkVMType.EVM || vmName === 'EVM' | |
| ? 'C' | |
| : vmName === NetworkVMType.PLATFORM || | |
| vmName === 'PLATFORM' || | |
| vmName === 'PlatformVM' || | |
| vmName === 'P' | |
| ? 'P' | |
| : 'X' // default to X-chain (AVM) for non-EVM, non-Platform VMs |
| useBalanceInCurrencyForAccount(account.id) | ||
| const { formatCurrency } = useFormatCurrency() | ||
| const accountData = useSelector(selectAccountById(account.id)) | ||
| const wallet = useSelector(selectWalletById(accountData?.walletId ?? '')) |
There was a problem hiding this comment.
The selectAccountById and selectWalletById selectors are called unconditionally on lines 128-129, but the account might not exist, and wallet might be undefined. While this is handled with optional chaining on line 129 (accountData?.walletId ?? ''), calling selectWalletById with an empty string when there's no wallet could be inefficient. Consider adding an early return or conditional rendering if accountData is undefined.
| const wallet = useSelector(selectWalletById(accountData?.walletId ?? '')) | |
| const walletId = accountData?.walletId | |
| const walletSelector = useMemo( | |
| () => (walletId ? selectWalletById(walletId) : () => undefined), | |
| [walletId] | |
| ) | |
| const wallet = useSelector(walletSelector) |
| // TODO: Consider using AnalyticsService here to track successful Ledger transactions | ||
| Logger.info('Ledger transaction completed') |
There was a problem hiding this comment.
The TODO comment on line 56 suggests adding AnalyticsService tracking for successful Ledger transactions. While this is already captured in the onDelegationSuccess callback in confirm.tsx (line 249 with 'StakeDelegationSuccess'), it would be beneficial to track Ledger-specific success events to distinguish between regular wallet and Ledger wallet staking operations for analytics purposes.
| } catch { | ||
| // If we can't parse the address (e.g., it's not a valid bech32), | ||
| // keep it as-is for backward compatibility |
There was a problem hiding this comment.
The normalizeXpAddressDict function on lines 27-45 catches all errors from formatAvalancheAddress and falls back to keeping the original address. However, this silent error handling could hide bugs where addresses are in an unexpected format. Consider logging a warning when an address cannot be normalized to help with debugging.
| } catch { | |
| // If we can't parse the address (e.g., it's not a valid bech32), | |
| // keep it as-is for backward compatibility | |
| } catch (error) { | |
| // If we can't parse the address (e.g., it's not a valid bech32), | |
| // keep it as-is for backward compatibility, but log a warning | |
| // to make unexpected address formats visible during debugging. | |
| console.warn( | |
| 'normalizeXpAddressDict: failed to normalize Avalanche address, keeping original address key', | |
| { address, isTestnet, error } | |
| ) |
| // For C chain (EVM): m/44'/60'/0' | ||
| const accountPath = | ||
| chainAlias === 'C' ? `m/44'/60'/0'` : `m/44'/9000'/${accountIndex}'` |
There was a problem hiding this comment.
The account path for C-chain on line 532 uses m/44'/60'/0' which is hardcoded to account index 0. This doesn't use the accountIndex parameter that was passed in. For C-chain transactions from different account indices, this could cause signature verification failures or signing with the wrong key. Consider using m/44'/60'/${accountIndex}' for consistency with X/P chain handling.
| // For C chain (EVM): m/44'/60'/0' | |
| const accountPath = | |
| chainAlias === 'C' ? `m/44'/60'/0'` : `m/44'/9000'/${accountIndex}'` | |
| // For C chain (EVM): m/44'/60'/{accountIndex}' | |
| const accountPath = | |
| chainAlias === 'C' ? `m/44'/60'/${accountIndex}'` : `m/44'/9000'/${accountIndex}'` |
| useEffect(() => { | ||
| if (approvalTriggeredRef.current) return | ||
|
|
||
| const handleApproveTransaction = async (): Promise<void> => { | ||
| if (deviceForWallet && isConnected) { | ||
| try { | ||
| approvalTriggeredRef.current = true | ||
| await LedgerService.openApp(ledgerAppName) | ||
| await onApprove() | ||
| } finally { | ||
| approvalTriggeredRef.current = false | ||
| if ( | ||
| deviceForWallet && | ||
| isConnected && | ||
| isAvalancheAppOpen && | ||
| phase === 'connection' && | ||
| onApprove | ||
| ) { | ||
| if (stakingProgress) { | ||
| // Create progress callback that updates local state | ||
| const onProgress = ( | ||
| step: number, | ||
| operation: Operation | null | ||
| ): void => { | ||
| setCurrentStep(step) | ||
| setCurrentOperation(operation) | ||
|
|
||
| // Auto-complete when all steps are done | ||
| if (step >= stakingProgress.totalSteps) { | ||
| setTimeout(() => { | ||
| stakingProgress.onComplete() | ||
| }, 500) // Brief delay to show final state | ||
| } | ||
| } | ||
| // Transition to progress phase | ||
| setPhase('progress') | ||
| // Start the transaction process with progress callback | ||
| onApprove(onProgress) | ||
| } else { | ||
| // No staking progress tracking, just approve and let the caller handle navigation | ||
| onApprove() | ||
| } | ||
| } | ||
| handleApproveTransaction() | ||
| }, [deviceForWallet, onApprove, isConnected, ledgerAppName]) | ||
| }, [ | ||
| deviceForWallet, | ||
| isConnected, | ||
| isAvalancheAppOpen, | ||
| phase, | ||
| stakingProgress, | ||
| onApprove | ||
| ]) |
There was a problem hiding this comment.
The useEffect on lines 158-198 will trigger onApprove every time the dependencies change while conditions are met. This could cause onApprove to be called multiple times if any of the dependencies (deviceForWallet, isConnected, isAvalancheAppOpen, phase, stakingProgress, onApprove) change. Consider adding a ref or state flag to ensure onApprove is only called once per connection session.
…acking step progress state for the ledgerReivewTransaction screen, decoupled delegation logic from ledger completely
…llet connect, matches logic in recent contacts where we only show wallet info if theres more than one wallet
B0Y3R-AVA
force-pushed
the
boyer/ledger-staking-delegation
branch
from
750ed8c to
0902a0c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if Avalanche app is open | ||
| const appType = LedgerService.getCurrentAppType() | ||
| const isAvaxApp = appType === LedgerAppType.AVALANCHE | ||
| setIsAvalancheAppOpen(isAvaxApp) |
There was a problem hiding this comment.
checkDeviceReady hard-codes LedgerAppType.AVALANCHE, but this modal is also used from ApprovalController for non-Avalanche Ledger signing. This will prevent the UI from ever transitioning out of the connection phase when ledgerAppName is ETHEREUM/SOLANA/BITCOIN/etc. Compare appType against the computed ledgerAppName instead of a constant (and update naming accordingly).
| }, | ||
| onReject: () => { | ||
| // User cancelled Ledger connection | ||
| Logger.info('Ledger transaction rejected') |
There was a problem hiding this comment.
executeLedgerStakingOperation passes an onReject that only logs. In LedgerReviewTransactionScreen, the Cancel/back/gesture handlers call onReject, so cancelling the Ledger modal during staking will not dismiss the modal or return to the previous screen. Make onReject close the modal (e.g., router.back() or stakingProgress.onCancel()).
| Logger.info('Ledger transaction rejected') | |
| Logger.info('Ledger transaction rejected') | |
| router.back() |
| await LedgerService.openApp(LedgerAppType.AVALANCHE) | ||
|
|
||
| // First ensure we're connected to the device | ||
| Logger.info('Ensuring connection to Ledger device...') | ||
| try { | ||
| await LedgerService.ensureConnection(this.deviceId) | ||
| Logger.info('Successfully connected to Ledger device') | ||
| } catch (error) { | ||
| Logger.error('Failed to connect to Ledger device:', error) | ||
| throw new Error( | ||
| 'Please make sure your Ledger device is nearby, unlocked, and Bluetooth is enabled.' | ||
| ) | ||
| ) { | ||
| throw new Error('Unable to sign avalanche transaction: invalid signer') | ||
| } | ||
|
|
||
| const txToSign = { | ||
| tx: transaction.tx, | ||
| externalIndices: transaction.externalIndices, | ||
| internalIndices: transaction.internalIndices | ||
| // Now ensure Avalanche app is ready | ||
| Logger.info('Ensuring Avalanche app is ready...') | ||
| try { | ||
| await LedgerService.waitForApp( | ||
| LedgerAppType.AVALANCHE, | ||
| LEDGER_TIMEOUTS.APP_WAIT_TIMEOUT | ||
| ) |
There was a problem hiding this comment.
LedgerService.openApp(...) is called before ensuring a transport connection. Since openApp requires an initialized transport (and is best-effort), calling it before ensureConnection makes it a no-op in the common case and reduces the chance of auto-opening the app. Reorder to ensureConnection → openApp → waitForApp, and consider reusing the transport returned by getTransport() to avoid double ensureConnection calls.
| xpAddressDictionary: XPAddressDictionary | ||
| } { | ||
| // Derive xpAddresses with fallback | ||
| // Note: All addresses should be stripped of HRP prefix for consistency with SDK expectations |
There was a problem hiding this comment.
The new comment says addresses are stripped of the "HRP prefix", but stripAddressPrefix only removes the chain prefix (X-/P-/C-). Update the comment to avoid confusion about what normalization is actually being applied.
| // Note: All addresses should be stripped of HRP prefix for consistency with SDK expectations | |
| // Note: All addresses should be normalized by stripping the chain prefix (e.g. X-/P-/C-) for consistency with SDK expectations |
4 participants
Description
Contributes to Ticket: CP-13198
Summary
Adds full Ledger hardware wallet support for the staking delegation flow, including a two-phase transaction modal and real-time progress
tracking.
Changes
New Files
LedgerStakingProgressScreen.tsx- Progress UI with animated dots showing current step (EXPORT_C → IMPORT_P → DELEGATE)ledgerStakingProgressCache.ts- In-memory cache for tracking progress state between componentswithLedgerStakingProgressCache.tsx- HOC wrapper for the progress screenledgerStakingProgress.tsx- New route for the progress modalModified Files
LedgerReviewTransactionScreen.tsxaddStake/confirm.tsxuseDelegation.tsLedgerWallet.tssignAvalancheTransaction()to use directAppAvax.sign()with device/app checkscomputeDelegationSteps/types.tsgetInternalExternalAddrs.tsledgerParamsCache.tsstakingProgressparams supportledger/utils/index.tsshowLedgerReviewTransaction()to accept staking progress configFeatures
Testing
iOS: 7349
Android: 7350
Screenshots/Videos
Screen.Recording.2026-01-29.at.4.52.33.PM.mov
Testing
Dev Testing (if applicable)
QA Testing (if applicable)
Checklist
Please check all that apply (if applicable)