feat(blockchain-link): add support for Solana v2 staking #17123
Conversation
tomasklim
requested review from
MiroslavProchazka,
marekrjpolak and
mroz22
as code owners
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
|
🚀 Expo preview is ready!
|
WalkthroughThis pull request introduces modifications across multiple packages to standardize dependency management, update type definitions, and refine transaction processing. Several ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/blockchain-link/src/workers/utils.ts (1)
50-70: Confirm robust indexing when accessingfieldsarray.While optional chaining protects against null references (
fields[1]?.delegation?.stake), consider clarifying assumptions around array lengths. This ensures readability and guards against unexpected changes iffieldsbecomes shorter.suite-common/wallet-utils/src/solanaStakingUtils.ts (1)
138-138: Consider removing spread from.reduceto improve performance.Using the spread operator in reducers (
(acc, [status, balance]) => ({ ...acc, [status]: balance })) can degrade performance for large arrays.A possible fix:
- (acc, [status, balance]) => ({ ...acc, [status]: balance }), + (acc, [status, balance]) => { + acc[status] = balance; + return acc; + },🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
packages/suite/src/utils/suite/solanaStaking.ts (2)
36-61: Refactor to avoid code duplication.
createTransactionShimCommonreplicates functionality from the connect package. Consider extracting shared logic into a single utility to reduce maintenance overhead.Would you like help restructuring this into a shared helper module?
124-128: Potential misprint in property name.
сomputeUnitPriceappears to use a non-ASCII “c” (could be Cyrillic).- сomputeUnitPrice: BigInt(estimatedFee.feePerUnit), + computeUnitPrice: BigInt(estimatedFee.feePerUnit),packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts (2)
77-126: Handle missing or unknown staking types ingetTransactionData.
Currently, the function silently returnsundefinedifstakeTypeis not'stake','unstake', or'claim'. Consider adding an else branch or a warning for unexpected stake types to improve debugging.+ else { + console.warn(`Unknown stake type: ${stakeType}`); + return; + }
128-152: Use optional chaining to simplify the fee payload check.
Replace the explicit checks with optional chaining to avoid repetitive null checks and align with best practices.- if (estimatedFee && estimatedFee.payload && 'levels' in estimatedFee.payload) { + if (estimatedFee?.payload?.levels) {🧰 Tools
🪛 Biome (1.9.4)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/blockchain-link-types/src/solana.ts (1)
73-77: NewSolanaStakingAccounttype is clear.
- The use of
bigintfor stake andrentExemptReservefields is appropriate for large numeric operations.- Consider enumerating possible
statusvalues if there's a known set of valid statuses.packages/suite/src/views/wallet/staking/components/StakingDashboard/components/StakingCard.tsx (1)
150-150: Consider inlining the network check.The
isEthereumNetworkboolean could be inlined where used to reduce variable declarations and improve code maintainability.-const isEthereumNetwork = selectedAccount.networkType === 'ethereum'; // Later in the code: -id={isEthereumNetwork ? 'TR_STAKE_APPROXIMATE_DAYS' : 'TR_UP_TO_DAYS'} +id={selectedAccount.networkType === 'ethereum' ? 'TR_STAKE_APPROXIMATE_DAYS' : 'TR_UP_TO_DAYS'}packages/blockchain-link-utils/src/blockbook.ts (1)
91-100:❓ Verification inconclusive
Document the purpose of these addresses and consider moving them to a configuration file.
The hardcoded Ethereum staking addresses should be:
- Documented to explain their purpose and usage.
- Moved to a configuration file for easier maintenance.
Run the following script to verify the validity of these Ethereum addresses:
🏁 Script executed:
#!/bin/bash # Description: Verify the validity of Ethereum addresses. # Test: Check if addresses follow the Ethereum address format (0x prefix and 40 hex characters). for addr in "0xD523794C879d9eC028960a231F866758e405bE34" "0xAFA848357154a6a624686b348303EF9a13F63264" "0x19449f0f696703Aa3b1485DfA2d855F33659397a" "0x66cb3AeD024740164EBcF04e292dB09b5B63A2e1"; do if ! echo "$addr" | grep -qE "^0x[a-fA-F0-9]{40}$"; then echo "Invalid Ethereum address format: $addr" exit 1 fi done # Test: Check if addresses are checksummed correctly using web3. python3 -c ' from web3 import Web3 addresses = [ "0xD523794C879d9eC028960a231F866758e405bE34", "0xAFA848357154a6a624686b348303EF9a13F63264", "0x19449f0f696703Aa3b1485DfA2d855F33659397a", "0x66cb3AeD024740164EBcF04e292dB09b5B63A2e1" ] for addr in addresses: if not Web3.is_checksum_address(addr): print(f"Address not checksummed correctly: {addr}") exit(1) 'Length of output: 1621
Action Required: Manually verify Ethereum address checksums and update documentation
- The regex check confirms the addresses match the expected Ethereum format.
- The automated checksum validation using the Web3 module failed due to a missing dependency, so please verify the checksum correctness manually.
- Document the purpose and usage of these hardcoded addresses and consider moving them to a configuration file for improved maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
package.json(1 hunks)packages/blockchain-link-types/package.json(0 hunks)packages/blockchain-link-types/src/solana.ts(1 hunks)packages/blockchain-link-utils/package.json(0 hunks)packages/blockchain-link-utils/src/blockbook.ts(1 hunks)packages/blockchain-link/package.json(1 hunks)packages/blockchain-link/src/workers/solana/index.ts(1 hunks)packages/blockchain-link/src/workers/utils.ts(3 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormActions.ts(3 hunks)packages/suite/src/actions/wallet/stake/stakeFormEthereumActions.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(8 hunks)packages/suite/src/components/suite/StakingProcess/StakingInfo.tsx(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/utils/suite/solanaStaking.ts(5 hunks)packages/suite/src/views/wallet/staking/components/StakingDashboard/components/StakingCard.tsx(2 hunks)suite-common/wallet-constants/src/solanaStakingConstants.ts(1 hunks)suite-common/wallet-utils/package.json(1 hunks)suite-common/wallet-utils/src/solanaStakingUtils.ts(5 hunks)
💤 Files with no reviewable changes (2)
- packages/blockchain-link-utils/package.json
- packages/blockchain-link-types/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
suite-common/wallet-utils/src/solanaStakingUtils.ts
[error] 138-138: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (23)
packages/blockchain-link/src/workers/utils.ts (2)
1-1: Good alignment with the Solana-specific SDK.Switching from
SolNetworktoNetworkis consistent with the project's move toward the dedicated Solana wallet SDK.
28-32: Ensure all call sites supply the new ‘epoch’ parameter.From a maintainability perspective, verify that external calls pass a valid integer for
epoch, and handle any potential mismatch or undefined behavior (e.g., negative values) accordingly.suite-common/wallet-utils/src/solanaStakingUtils.ts (2)
1-1: Migration to the dedicated Solana SDK looks good.Dropping
@everstake/wallet-sdkin favor of@everstake/wallet-sdk-solanaaligns with the new specialized approach for Solana-specific functionality.
35-35: Network property update is correct.Replacing the older
SolNetworktype with the newNetworktype is consistent and keeps the interface accurate following the SDK migration.packages/suite/src/utils/suite/solanaStaking.ts (2)
26-34: SolanaTx and TransactionShim definitions are logically separated.Defining a specialized shim for transaction manipulation keeps concerns well-organized and clarifies the transaction’s lifecycle.
119-129: Consider validating fee parameters.When
estimatedFeeis provided, ensure it’s within sensible bounds. Large or invalid values could cause unexpected transaction costs.packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts (5)
1-2: No concerns with new import usage.
Theaddressimport from@solana/web3.jsis correctly utilized later for signature addition.
13-23: Imports look good, ensuring proper type usage.
All newly introduced or re-imported types (Account,BlockchainNetworks,SelectedAccountStatus,Fee) are coherent with the updated logic.
38-75: Check fallback fee consistency incalculateTransaction.
- Using
estimatedFee?.feePerTx ?? new BigNumber(SOL_STAKING_OPERATION_FEE).toString()is a suitable fallback. Confirm thatSOL_STAKING_OPERATION_FEEconsistently represents lamports to avoid discrepancies in fee calculations.- Merging
feeLevelwithestimatedFee(line 66) may overshadow some properties. Verify it is intended behavior.
155-185: Deferred fee estimation logic is well-structured.
The async logic for callingestimateFeebefore composing the transaction is clear and maintainable. Great job!
204-215: Verify signature method on line 264.
Passingaddress(account.descriptor)is a valid approach to derive a Solana-compatible key. However, ensure thatdescriptoralways yields a base58-encoded address and not an extended account path.Also applies to: 264-266
suite-common/wallet-constants/src/solanaStakingConstants.ts (1)
6-10: Verify increased withdrawal minimum and new compute constants.
- The jump of
MIN_SOL_FOR_WITHDRAWALSfrom 0.000005 to 0.05 is substantial; confirm that it matches your business logic and won’t block users with smaller balances.- The newly introduced
SOL_COMPUTE_UNIT_PRICEandSOL_COMPUTE_UNIT_LIMITexpand the staking computation framework; ensure they are appropriately applied in fee calculations.packages/suite/src/components/suite/StakingProcess/StakingInfo.tsx (1)
39-42: LGTM! Improved clarity in staking duration display.The change from
TR_STAKE_DAYStoTR_STAKE_APPROXIMATE_DAYSbetter reflects the approximate nature of the staking duration, improving user understanding.packages/suite/src/actions/wallet/stake/stakeFormActions.ts (1)
15-15: LGTM! Enhanced fee estimation support.The addition of the optional
estimatedFeeparameter improves transaction composition by allowing for more accurate fee estimation while maintaining backward compatibility.Also applies to: 119-121, 135-142
packages/suite/src/actions/wallet/stake/stakeFormEthereumActions.ts (1)
98-98: LGTM! Maintained type safety with new parameter.The explicit
undefinedparameter properly adapts to the updatedcomposeStakingTransactionsignature while maintaining type safety.packages/suite/src/views/wallet/staking/components/StakingDashboard/components/StakingCard.tsx (1)
228-238: LGTM! Improved network-specific messaging.The conditional translation ID selection provides better network-specific messaging for unstaking duration display.
packages/blockchain-link-utils/src/blockbook.ts (1)
102-106: LGTM! The refactoring improves code readability.The function has been simplified while maintaining its original functionality. The use of array
includesmethod makes the code more readable and maintainable.packages/blockchain-link/src/workers/solana/index.ts (1)
369-374:✅ Verification successful
LGTM! The epoch caching implementation looks good.
The changes correctly integrate epoch information into the staking data retrieval process. The caching of epoch data (3600s) is appropriate for Solana's epoch duration.
Run the following script to verify the epoch caching behavior:
Also applies to: 378-379
🏁 Script executed:
#!/bin/bash # Description: Verify that epoch caching is working correctly. # Test: Search for other epoch cache durations in the codebase to ensure consistency. rg -A 2 'cache\.set.*epoch' # Test: Search for epoch-related constants to ensure the cache duration aligns with Solana's epoch duration. rg 'EPOCH|epoch.*duration'Length of output: 4104
Epoch Caching Implementation Verified and Approved
- The caching duration is correctly set to 3,600,000 ms, aligning with Solana’s epoch requirements.
- The epoch is fetched via
getEpochand passed appropriately togetSolanaStakingData.- Consistency across related modules was confirmed, ensuring the caching and epoch logic is sound.
LGTM!
packages/suite/src/support/messages.ts (1)
8947-8950: LGTM! The new message definition follows best practices.The new
TR_STAKE_APPROXIMATE_DAYSmessage is well-structured with proper ICU MessageFormat syntax for pluralization and maintains consistency with existing message patterns.suite-common/wallet-utils/package.json (1)
15-15: Dependency Update for Solana SDK:
The addition of@everstake/wallet-sdk-solanaat version2.0.5replaces the generic wallet SDK dependency. This change aligns with the objective of providing Solana-specific functionality. Please ensure that any parts of the codebase that previously relied on@everstake/wallet-sdkare updated accordingly.packages/blockchain-link/package.json (1)
75-75: Transition to Solana SDK:
The introduction of@everstake/wallet-sdk-solana(version2.0.5) in the dependencies is consistent with the PR’s aim to support Solana v2 staking. Please verify that downstream modules and transaction processing logic correctly integrate with this new dependency.packages/suite/package.json (1)
21-21: Strict Version Locking of Wallet SDK:
The dependency for@everstake/wallet-sdkis now locked to version1.0.10rather than using a caret version. This strict version locking can ensure consistent behavior but may prevent receiving minor updates automatically. Please verify that this decision fits your long-term compatibility requirements, especially given the broader refactoring toward Solana-specific SDKs in related packages.package.json (1)
103-103: Resolution Entry Update:
The resolution for"promise"is now explicitly set to"8.3.0", and the previous resolution entry for@everstake/wallet-sdk/@solana/web3.jshas been removed. This simplifies dependency management; however, please verify that this change does not introduce any conflicts—particularly in mobile builds or other environments where precise version matching might be critical.
tomasklim
added
the
no-project
|
@SocketSecurity ignore npm/@everstake/wallet-sdk-solana@2.0.5 |
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13437921712 |
trezor-ci
force-pushed
the
test-everstake
branch
from
60ffe37 to
31a8309
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts (4)
38-75: Add validation for estimatedFee parameter.While the fee handling logic is correct, consider adding validation to ensure the estimatedFee values are within acceptable ranges to prevent potential issues with extreme values.
const calculateTransaction = ( availableBalance: string, output: ExternalOutput, feeLevel: FeeLevel, compareWithAmount = true, symbol: NetworkSymbol, estimatedFee?: Fee[number], ): PrecomposedTransaction => { + // Validate estimated fee is within acceptable range + if (estimatedFee?.feePerTx) { + const minFee = new BigNumber(0); + const maxFee = new BigNumber(SOL_STAKING_OPERATION_FEE).multipliedBy(10); // 10x default as safety margin + const estimatedFeeBN = new BigNumber(estimatedFee.feePerTx); + if (estimatedFeeBN.lt(minFee) || estimatedFeeBN.gt(maxFee)) { + estimatedFee = undefined; // Fall back to default fee + } + } + const feeInLamports = estimatedFee?.feePerTx ?? new BigNumber(SOL_STAKING_OPERATION_FEE).toString();
77-126: Enhance error handling in getTransactionData.The function has good type checking but could benefit from explicit error handling for edge cases.
const getTransactionData = async ( formValues: StakeFormState, selectedAccount: SelectedAccountStatus, blockchain: BlockchainNetworks, estimatedFee?: Fee[number], ) => { const { stakeType } = formValues; - if (selectedAccount.status !== 'loaded') return; + if (selectedAccount.status !== 'loaded') { + throw new Error('Account not loaded'); + } const { account } = selectedAccount; - if (account.networkType !== 'solana') return; + if (account.networkType !== 'solana') { + throw new Error('Invalid network type'); + } const selectedBlockchain = blockchain[account.symbol]; + if (!selectedBlockchain) { + throw new Error('Blockchain not found'); + }
128-152: Simplify null checks using optional chaining.The function's null checks can be simplified using optional chaining.
async function estimateFee( account: Account, txData?: PrepareStakeSolTxResponse, ): Promise<Fee[number] | undefined> { if (!txData?.success) return undefined; const estimatedFee = await TrezorConnect.blockchainEstimateFee({ coin: account.symbol, request: { specific: { data: txData.tx.txShim.serialize(), isCreatingAccount: false, newTokenAccountProgramName: undefined, }, }, }); - if (estimatedFee && estimatedFee.payload && 'levels' in estimatedFee.payload) { - const { levels } = estimatedFee.payload; - - return levels[0]; - } + return estimatedFee?.payload?.levels?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
154-184: Add error handling for async operations.The async operations could benefit from proper error handling to prevent unhandled rejections.
export const composeTransaction = (formValues: StakeFormState, formState: ComposeActionContext) => async (_: Dispatch, getState: GetState) => { + try { const { selectedAccount, blockchain } = getState().wallet; if (selectedAccount.status !== 'loaded') return; const { account } = selectedAccount; const txData = await getTransactionData(formValues, selectedAccount, blockchain); let estimatedFee; // it is not needed to estimate fee for empty input if (formValues.cryptoInput) { estimatedFee = await estimateFee(account, txData); } const { feeInfo } = formState; if (!feeInfo) return; const { levels } = feeInfo; const predefinedLevels = levels.filter(l => l.label !== 'custom'); return composeStakingTransaction( formValues, formState, predefinedLevels, calculateTransaction, estimatedFee, undefined, ); + } catch (error) { + console.error('Error composing transaction:', error); + return undefined; + } };packages/suite/src/utils/suite/solanaStaking.ts (3)
1-34: LGTM! Well-structured imports and type definitions.The imports are well-organized, and the types are clearly defined. The new
TransactionShimtype provides essential methods for transaction handling.Consider adding JSDoc comments for the
TransactionShimtype to document the purpose of each method:type TransactionShim = { + /** Adds a signature to the transaction using the signer's public key and signature in hex format */ addSignature(signerPubKey: string, signatureHex: string): void; + /** Returns the serialized transaction message */ serializeMessage(): string; + /** Returns the fully serialized transaction including signatures */ serialize(): string; };
36-61: Track technical debt: Refactor duplicated code.The TODO comment indicates code duplication with the connect package. While the implementation is correct, this should be tracked and addressed to maintain DRY principles.
Would you like me to create an issue to track this technical debt? The issue would focus on:
- Moving the common code to a shared location
- Updating both packages to use the shared implementation
- Removing the duplicated code
146-148: Enhance error message for better debugging.The error message could be more descriptive to help with debugging.
Apply this diff to improve the error message:
- throw new Error('Transaction is not compilable'); + throw new Error('Transaction validation failed: Expected a CompilableTransactionMessage but received an incompatible transaction type');packages/blockchain-link/src/workers/utils.ts (1)
50-70: Enhance error handling with specific error messages.While the implementation correctly handles stake accounts and filters undefined values, the error handling could be more informative for debugging purposes.
Consider adding specific error messages for different failure cases:
return stakingAccounts .map(account => { const stakeAccount = account?.data; - if (!stakeAccount) return; + if (!stakeAccount) { + console.warn('Skipping account: Missing stake account data'); + return; + } const stakeState = stakeAccountState(stakeAccount, BigInt(epoch)); const { state } = account?.data ?? {}; - if (!isStake(state)) return; + if (!isStake(state)) { + console.warn('Skipping account: Invalid stake state'); + return; + }suite-common/wallet-utils/src/solanaStakingUtils.ts (2)
56-59: Add type checking for stake amount.While the code is more readable, it could benefit from explicit type checking for the stake amount.
Consider adding explicit type checking:
const totalAmount = stakingAccounts.reduce((acc, account) => { if (account) { - const delegationStake = account?.stake?.toString(); + const stake = account.stake; + if (typeof stake === 'bigint' || typeof stake === 'number' || typeof stake === 'string') { + const delegationStake = stake.toString();
97-104: Add type guard for staking account status.While the status filtering works, it could benefit from type safety improvements.
Consider adding a type guard:
+const isValidStakeState = (status: string): status is StakeStateType => + Object.values(StakeState).includes(status as StakeState); export const getSolanaStakingAccountsByStatus = (account: Account, status: string) => { if (account?.networkType !== 'solana') return []; const { solStakingAccounts } = account?.misc ?? {}; if (!solStakingAccounts) return []; + if (!isValidStakeState(status)) { + console.warn(`Invalid stake state: ${status}`); + return []; + } return solStakingAccounts.filter(solStakingAccount => solStakingAccount.status === status); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
package.json(1 hunks)packages/blockchain-link-types/package.json(0 hunks)packages/blockchain-link-types/src/solana.ts(1 hunks)packages/blockchain-link-utils/package.json(0 hunks)packages/blockchain-link-utils/src/blockbook.ts(1 hunks)packages/blockchain-link/package.json(1 hunks)packages/blockchain-link/src/workers/solana/index.ts(1 hunks)packages/blockchain-link/src/workers/utils.ts(3 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormActions.ts(3 hunks)packages/suite/src/actions/wallet/stake/stakeFormEthereumActions.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(8 hunks)packages/suite/src/components/suite/StakingProcess/StakingInfo.tsx(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/utils/suite/solanaStaking.ts(5 hunks)packages/suite/src/views/wallet/staking/components/StakingDashboard/components/StakingCard.tsx(2 hunks)scripts/list-outdated-dependencies/trends-dependencies.txt(1 hunks)suite-common/wallet-constants/src/solanaStakingConstants.ts(1 hunks)suite-common/wallet-utils/package.json(1 hunks)suite-common/wallet-utils/src/solanaStakingUtils.ts(5 hunks)
💤 Files with no reviewable changes (2)
- packages/blockchain-link-utils/package.json
- packages/blockchain-link-types/package.json
🚧 Files skipped from review as they are similar to previous changes (14)
- scripts/list-outdated-dependencies/trends-dependencies.txt
- suite-common/wallet-utils/package.json
- packages/blockchain-link-types/src/solana.ts
- packages/suite/src/components/suite/StakingProcess/StakingInfo.tsx
- package.json
- packages/blockchain-link/package.json
- packages/suite/src/actions/wallet/stake/stakeFormEthereumActions.ts
- packages/suite/src/views/wallet/staking/components/StakingDashboard/components/StakingCard.tsx
- packages/blockchain-link-utils/src/blockbook.ts
- suite-common/wallet-constants/src/solanaStakingConstants.ts
- packages/suite/package.json
- packages/suite/src/actions/wallet/stake/stakeFormActions.ts
- packages/blockchain-link/src/workers/solana/index.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
suite-common/wallet-utils/src/solanaStakingUtils.ts
[error] 138-138: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (6)
packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts (1)
264-266: Verify signature before serialization.The signature should be verified before serialization to ensure its validity and prevent potential security issues.
- txData.tx.txShim.addSignature(address(account.descriptor), signedTx.payload.signature); - - return txData.tx.txShim.serialize(); + const signature = signedTx.payload.signature; + // Verify signature before adding it + if (!txData.tx.txShim.verifySignature(address(account.descriptor), signature)) { + throw new Error('Invalid signature'); + } + txData.tx.txShim.addSignature(address(account.descriptor), signature); + + return txData.tx.txShim.serialize();packages/suite/src/utils/suite/solanaStaking.ts (1)
63-83: LGTM! Improved type safety in transaction transformation.The function correctly handles transaction compilation and transformation using the new transaction types from
@solana/web3.js.packages/blockchain-link/src/workers/utils.ts (2)
1-1: LGTM! Import statement updated for Solana v2 SDK.The import statement has been correctly updated to use the new Solana-specific SDK, which aligns with the PR's objective of adding Solana v2 staking support.
28-41: LGTM! Function signature and network type updated for Solana v2.The changes correctly:
- Add the required
epochparameter for stake account state determination- Update network type to use the new SDK's
Networkenumsuite-common/wallet-utils/src/solanaStakingUtils.ts (2)
1-1: LGTM! Import and type definitions updated for Solana v2.The changes correctly update the imports and network type definitions to use the new Solana SDK.
Also applies to: 34-36
38-45: LGTM! Network configuration updated for Solana v2.The network configuration correctly maps network symbols to the new SDK's
Networkenum values.
| const getStakingParams = (estimatedFee?: Fee[number]) => { | ||
| if (!estimatedFee || !estimatedFee.feePerUnit || !estimatedFee.feeLimit) { | ||
| return dummyPriorityFeesForFeeEstimation; | ||
| } | ||
|
|
||
| return { | ||
| сomputeUnitPrice: BigInt(estimatedFee.feePerUnit), | ||
| computeUnitLimit: Number(estimatedFee.feeLimit), // solana package expects number | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Fix non-ASCII character in variable name.
The variable сomputeUnitPrice contains a non-ASCII 'c' character which could cause issues.
Apply this diff to fix the variable name:
return {
- сomputeUnitPrice: BigInt(estimatedFee.feePerUnit),
+ computeUnitPrice: BigInt(estimatedFee.feePerUnit),
computeUnitLimit: Number(estimatedFee.feeLimit), // solana package expects number
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getStakingParams = (estimatedFee?: Fee[number]) => { | |
| if (!estimatedFee || !estimatedFee.feePerUnit || !estimatedFee.feeLimit) { | |
| return dummyPriorityFeesForFeeEstimation; | |
| } | |
| return { | |
| сomputeUnitPrice: BigInt(estimatedFee.feePerUnit), | |
| computeUnitLimit: Number(estimatedFee.feeLimit), // solana package expects number | |
| }; | |
| }; | |
| const getStakingParams = (estimatedFee?: Fee[number]) => { | |
| if (!estimatedFee || !estimatedFee.feePerUnit || !estimatedFee.feeLimit) { | |
| return dummyPriorityFeesForFeeEstimation; | |
| } | |
| return { | |
| computeUnitPrice: BigInt(estimatedFee.feePerUnit), | |
| computeUnitLimit: Number(estimatedFee.feeLimit), // solana package expects number | |
| }; | |
| }; |
| const balances: Record<StakeStateType, string> = balanceResults.reduce( | ||
| (acc, [key, balance]) => ({ ...acc, [key]: balance }), | ||
| (acc, [status, balance]) => ({ ...acc, [status]: balance }), | ||
| {}, | ||
| ); |
There was a problem hiding this comment.
Optimize performance by avoiding spread operator in reduce.
The use of spread operator in the reduce accumulator can lead to O(n²) complexity.
Consider using Object.fromEntries for better performance:
- const balances: Record<StakeStateType, string> = balanceResults.reduce(
- (acc, [status, balance]) => ({ ...acc, [status]: balance }),
- {},
- );
+ const balances = Object.fromEntries(balanceResults) as Record<StakeStateType, string>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const balances: Record<StakeStateType, string> = balanceResults.reduce( | |
| (acc, [key, balance]) => ({ ...acc, [key]: balance }), | |
| (acc, [status, balance]) => ({ ...acc, [status]: balance }), | |
| {}, | |
| ); | |
| const balances = Object.fromEntries(balanceResults) as Record<StakeStateType, string>; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
tomasklim
force-pushed
the
test-everstake
branch
from
31a8309 to
7670a6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
suite-common/wallet-utils/src/solanaStakingUtils.ts (1)
137-140: 🛠️ Refactor suggestionOptimize performance by avoiding spread operator in reduce.
The use of spread operator in the reduce accumulator can lead to O(n²) complexity.
Apply this diff to improve performance:
- const balances: Record<StakeStateType, string> = balanceResults.reduce( - (acc, [status, balance]) => ({ ...acc, [status]: balance }), - {}, - ); + const balances = Object.fromEntries(balanceResults) as Record<StakeStateType, string>;🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
packages/suite/src/utils/suite/solanaStaking.ts (1)
119-128:⚠️ Potential issueFix non-ASCII character in variable name.
The variable
сomputeUnitPricecontains a non-ASCII 'c' character which could cause issues.Apply this diff to fix the variable name:
return { - сomputeUnitPrice: BigInt(estimatedFee.feePerUnit), + computeUnitPrice: BigInt(estimatedFee.feePerUnit), computeUnitLimit: Number(estimatedFee.feeLimit), // solana package expects number };
🧹 Nitpick comments (1)
packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts (1)
128-152: Simplify conditional check using optional chaining.The conditional check in the
estimateFeefunction can be simplified.Apply this diff to improve readability:
- if (estimatedFee && estimatedFee.payload && 'levels' in estimatedFee.payload) { + if (estimatedFee?.payload?.levels) { const { levels } = estimatedFee.payload; return levels[0]; }🧰 Tools
🪛 Biome (1.9.4)
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
package.json(1 hunks)packages/blockchain-link-types/package.json(0 hunks)packages/blockchain-link-types/src/solana.ts(1 hunks)packages/blockchain-link-utils/package.json(0 hunks)packages/blockchain-link-utils/src/blockbook.ts(1 hunks)packages/blockchain-link/package.json(1 hunks)packages/blockchain-link/src/workers/solana/index.ts(1 hunks)packages/blockchain-link/src/workers/utils.ts(3 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormActions.ts(3 hunks)packages/suite/src/actions/wallet/stake/stakeFormEthereumActions.ts(1 hunks)packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts(8 hunks)packages/suite/src/components/suite/StakingProcess/StakingInfo.tsx(1 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/utils/suite/solanaStaking.ts(5 hunks)packages/suite/src/views/wallet/staking/components/StakingDashboard/components/StakingCard.tsx(2 hunks)scripts/list-outdated-dependencies/trends-dependencies.txt(1 hunks)suite-common/wallet-constants/src/solanaStakingConstants.ts(1 hunks)suite-common/wallet-utils/package.json(1 hunks)suite-common/wallet-utils/src/solanaStakingUtils.ts(5 hunks)
💤 Files with no reviewable changes (2)
- packages/blockchain-link-utils/package.json
- packages/blockchain-link-types/package.json
🚧 Files skipped from review as they are similar to previous changes (14)
- scripts/list-outdated-dependencies/trends-dependencies.txt
- packages/suite/package.json
- packages/suite/src/components/suite/StakingProcess/StakingInfo.tsx
- package.json
- packages/blockchain-link/package.json
- suite-common/wallet-utils/package.json
- packages/suite/src/views/wallet/staking/components/StakingDashboard/components/StakingCard.tsx
- packages/blockchain-link-types/src/solana.ts
- packages/blockchain-link-utils/src/blockbook.ts
- suite-common/wallet-constants/src/solanaStakingConstants.ts
- packages/blockchain-link/src/workers/solana/index.ts
- packages/suite/src/actions/wallet/stake/stakeFormEthereumActions.ts
- packages/suite/src/actions/wallet/stake/stakeFormActions.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts
[error] 145-145: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
suite-common/wallet-utils/src/solanaStakingUtils.ts
[error] 138-138: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Linting and formatting
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (10)
packages/blockchain-link/src/workers/utils.ts (4)
1-1: LGTM!The import statement correctly imports the required functions from the Solana-specific wallet SDK.
28-32: LGTM!The function signature has been updated to include the
epochparameter, which is used to determine stake state.
40-40: LGTM!The network assignment correctly uses the new
Networkenum from the Solana-specific wallet SDK.
50-70: LGTM!The function correctly processes staking accounts and filters out undefined values. The code is well-structured and handles edge cases appropriately.
suite-common/wallet-utils/src/solanaStakingUtils.ts (3)
1-1: LGTM!The import statement correctly imports the required types from the Solana-specific wallet SDK.
34-36: LGTM!The interface correctly uses the new
Networktype from the Solana-specific wallet SDK.
40-42: LGTM!The network assignments correctly use the new
Networkenum values.packages/suite/src/utils/suite/solanaStaking.ts (2)
1-10: LGTM!The import statements correctly import the required transaction types and utilities.
30-34: LGTM!The
TransactionShimtype correctly defines the required methods for transaction manipulation.packages/suite/src/actions/wallet/stake/stakeFormSolanaActions.ts (1)
77-126: LGTM!The
getTransactionDatafunction correctly handles different staking types and passes the estimated fee to the transaction preparation functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/test-utils/src/expoMock.js (1)
3-7: Enhance the setImmediate polyfill implementation.While the implementation is functionally correct, consider these improvements for better robustness:
- Add type checking for the callback parameter
- Return the timeout ID to maintain compatibility with the native
setImmediate- Add error handling for invalid callbacks
if (!window.setImmediate) { - window.setImmediate = function (callback) { - setTimeout(callback, 0); + window.setImmediate = function (callback) { + if (typeof callback !== 'function') { + throw new TypeError('Callback must be a function'); + } + return setTimeout(callback, 0); }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-native/test-utils/src/expoMock.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze with CodeQL (javascript)
Description
Related Issue
Resolve
Screenshots: