feat(e2e): New Trading test buy solana #16742
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
error Error: http://10.0.0.28:4873/@trezor%2feslint: no such package available WalkthroughThe pull request introduces comprehensive changes to the cryptocurrency trading functionality, specifically focusing on expanding support for Solana token transactions. The modifications span multiple files within the e2e testing infrastructure, including new JSON fixtures for Solana quotes and trade data, updated type definitions, and enhanced test suites. The changes introduce a more modular approach to handling cryptocurrency trades by creating specific implementations for Bitcoin and Solana transactions. This includes adding new methods in page action classes, updating import structures, and creating detailed type definitions to support more granular trade processing. The test suites have been expanded to include specific scenarios for Solana trading, with new mock data and validation steps to ensure robust testing of the trading functionality across different cryptocurrency networks. The modifications also include improvements to wallet management, such as adding a new method for handling unused hidden wallets, which provides more flexibility in wallet creation and management within the testing environment. ✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (9)
packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts (2)
238-246: New 'setYouPayCryptoAmount' step broadens test coverage for crypto inputs.
Consider parameterizing the crypto symbol if future expansions require multiple crypto assets. Otherwise, this approach is consistent with your existing fiat-input logic.
314-314: Filtering with 'buyQuotesBTC' is valid for BTC tests.
If you plan to add dynamic filtering for multiple assets, consider introducing a more generic approach to handle different quote sets.packages/suite-desktop-core/e2e/fixtures/invity/types.ts (1)
1-26: Comprehensive 'Trade' type.
To enhance type safety, consider using an enum forstatusif it has a limited set of possible values.packages/suite-desktop-core/e2e/fixtures/invity/index.ts (2)
44-44: Referencing 'buyQuotesBTC' in 'invityResponses' is consistent with the new naming convention.
For future multi-asset expansions, you might create a more dynamic or symbolic mapping.
50-56: Enhanced 'createRedirectedTradeResponse' with structured params.
Consider verifying thatsymbolaligns withtradeRequest.trade.receiveCurrencyortradeRequest.trade.paymentIdto avoid unintentional mismatches.packages/suite-desktop-core/e2e/tests/coin-market/buy-solana.test.ts (2)
8-13: Consider extracting test constants to a separate file.These test constants are similar to those in buy-coins.test.ts. Consider extracting them to a shared constants file to improve maintainability and reusability.
51-61: Consider adding error scenarios.While the happy path is well tested, consider adding test cases for:
- Network errors during trade confirmation
- Invalid receive address
- Payment method unavailability
packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts (1)
35-40: Consider parameterizing the offer selection logic.The hardcoded index (5) for the second offer makes the test brittle. Consider parameterizing this or adding a helper method to find a suitable offer based on criteria.
-// secondOffer that matches input criteria has index 5 -const secondOfferProvider = capitalizeFirstLetter(buyQuotesBTC[5].exchange); +const findOfferByIndex = (quotes, index) => quotes[index]; +const secondOffer = findOfferByIndex(buyQuotesBTC, 5); +const secondOfferProvider = capitalizeFirstLetter(secondOffer.exchange);packages/suite-desktop-core/e2e/fixtures/invity/buy/quotes-solana.json (1)
12-13: Standardize decimal places in string amounts.The
receiveStringAmountvalues have inconsistent decimal places:
- Some use 8 decimal places: "0.50000000"
- Others use 9 decimal places: "0.457812697"
- Some use just 1 decimal place: "0.5"
Consider standardizing the decimal places for consistency.
Also applies to: 32-33, 52-53, 72-73, 92-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/suite-desktop-core/e2e/snapshots/web/coin-market/buy-coins.test.ts/buy-coins-layout.pngis excluded by!**/*.pngpackages/suite-desktop-core/e2e/snapshots/web/coin-market/buy-coins.test.ts/compared-offers-buy-confirmation.pngis excluded by!**/*.pngpackages/suite-desktop-core/e2e/snapshots/web/coin-market/buy-coins.test.ts/transactions-detail.pngis excluded by!**/*.png
📒 Files selected for processing (10)
packages/suite-desktop-core/e2e/fixtures/invity/buy/quotes-solana.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/buy/trade-solana.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/index.ts(3 hunks)packages/suite-desktop-core/e2e/fixtures/invity/types.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/dashboardActions.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts(6 hunks)packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/coin-market/buy-solana.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/metadata/metadata-lifecycle.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/metadata/wallet-metadata.test.ts(2 hunks)
🔇 Additional comments (21)
packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts (5)
13-13: Renaming variable to 'buyQuotesBTC' looks consistent.
This clarifies the scope of these quotes specifically for BTC and aligns with the addition of separate SOL quotes.
17-17: Importing TradeRequest keeps the typed approach consistent.
The newly introduced type usage ensures strongly typed and predictable data structures.
95-95: Adding the 'confirmationPaymentMethod' locator is a good addition.
This new locator makes the payment method element accessible for accurate verification in tests.
160-160: Assigning the confirmationPaymentMethod to the correct test ID.
No issues: The locator correctly references the intended element for test validation.
270-283: 'mockInvityTrade' neatly bypasses the provider flow for streamlined test scenarios.
As a precaution, verify that parallel tests don't overwrite or conflict with each other's mock routes.packages/suite-desktop-core/e2e/fixtures/invity/types.ts (2)
28-34: 'TradeForm' structure is straightforward and clearly separates form data.
No issues found.
36-39: Exporting 'TradeRequest' provides a clean, unified interface.
No issues found.packages/suite-desktop-core/e2e/fixtures/invity/index.ts (4)
3-4: Importing 'NetworkSymbol' sets up typed usage of network identifiers.
No issues found here.
12-15: Separate JSON imports for BTC and SOL quotes and trades are well-structured.
This modular approach prepares the tests for multiple cryptocurrencies.
18-18: TradeRequest import aligns with typed request handling.
No issues found.
71-74: Exporting SOL references complements the BTC references.
This keeps expansions organized for upcoming cryptocurrency support.packages/suite-desktop-core/e2e/tests/metadata/metadata-lifecycle.test.ts (1)
57-57: LGTM! Method name change improves clarity.The rename from
addHiddenWallettoaddUnusedHiddenWalletbetter describes the wallet's state and makes the code more self-documenting.packages/suite-desktop-core/e2e/tests/coin-market/buy-solana.test.ts (3)
42-49: LGTM! Comprehensive test coverage for crypto amount input.The test properly verifies:
- Amount input handling
- Best offer display
- Provider information
- Quote selection
63-68: LGTM! Thorough transaction verification.The test properly verifies all important aspects of the transaction:
- Status
- Amounts
- Provider information
34-40: Verify network enablement persistence.The test enables the Solana network but doesn't verify if this setting persists across sessions. Consider adding a test case to verify network settings persistence.
packages/suite-desktop-core/e2e/tests/coin-market/buy-coins.test.ts (1)
9-14: LGTM! Clear variable naming for Bitcoin-specific tests.The renaming of variables (e.g.,
fiatAmount,bestBuyProvider) makes it clear these are Bitcoin-specific tests, which is important now that Solana tests exist.packages/suite-desktop-core/e2e/tests/metadata/wallet-metadata.test.ts (1)
45-45: LGTM! Consistent method naming across test files.The rename to
addUnusedHiddenWalletis consistently applied across test files, improving code clarity and maintainability.Also applies to: 96-96
packages/suite-desktop-core/e2e/support/pageActions/dashboardActions.ts (1)
Line range hint
105-119: LGTM! Well-structured implementation.The new
addUnusedHiddenWalletmethod is well-implemented, building upon the existingaddHiddenWalletmethod and following a clear sequence of steps for wallet confirmation.packages/suite-desktop-core/e2e/fixtures/invity/buy/trade-solana.json (2)
13-13: Verify expiration timestamp handling.The expiration timestamp is set to 2025. Ensure the application correctly handles far-future expiration dates and consider using a shorter timeframe for testing.
7-7: Consider URL parameter security.The URLs contain sensitive data (payment IDs, order IDs, signatures) that could be exposed in logs. Consider:
- Using placeholder values in test fixtures
- Implementing URL parameter masking in logs
Also applies to: 31-31
✅ Verification successful
Test fixtures with sensitive-looking data are acceptable in this context
The review concern can be dismissed because:
- The URL is contained within a test fixture and is not exposed in application logs
- The data includes an expiration timestamp, limiting its validity
- Test fixtures need realistic data to properly validate application behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if sensitive URL parameters are being logged # Search for logging statements that might expose these URLs rg -A 2 "trezor\.banxa\.com\?expires=" --type tsLength of output: 50
Script:
#!/bin/bash # Search for imports or usage of the trade-solana fixture rg -l "trade-solana\.json" --type ts # Look for URL logging patterns in test files rg -A 2 "console\.(log|debug|info)" "packages/suite-desktop-core/e2e/" # Search for general URL parameter logging rg -A 2 "(url|URL).*(log|console)" --type tsLength of output: 3512
packages/suite-desktop-core/e2e/fixtures/invity/buy/quotes-solana.json (1)
158-158: Ensure unique quote IDs across payment methods.The same quote ID "2dabc78d-e291-4fed-973d-c484ac0a3bee" is used for different payment methods (PayPal, Google Pay, Credit Card, Apple Pay) from the same exchange (paybis). This could cause issues with quote tracking and order processing.
Also applies to: 237-237, 314-314, 334-334
| "error": "This quote is unavailable at the moment.", | ||
| "exchange": "moonpay", | ||
| "fiatStringAmount": "", | ||
| "receiveStringAmount": "", | ||
| "minFiat": 500, | ||
| "maxFiat": 260000, | ||
| "minCrypto": 0.02, | ||
| "maxCrypto": 30000, | ||
| "paymentMethod": "paypal", | ||
| "quoteId": "eded8429-8e0b-4278-9013-f4f04827ac4d" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error quote structure.
The error quote object has a different structure from success quotes. Consider:
- Including common fields like
fiatCurrency,receiveCurrency,rate, etc. - Using null/empty values instead of omitting fields
- Adding an error code for better error handling
This will make parsing and handling quotes more consistent.
There was a problem hiding this comment.
This is 3rd party response provided by invity.
There was a problem hiding this comment.
@Vere-Grey Understood, I apologize for the suggestion. Since it's a 3rd party response, we don't have control over its structure.
✏️ Learnings added
Learnt from: Vere-Grey
PR: trezor/trezor-suite#16742
File: packages/suite-desktop-core/e2e/fixtures/invity/buy/quotes-solana.json:436-446
Timestamp: 2025-01-30T21:30:59.129Z
Learning: The quote responses in the Invity API are controlled by the 3rd party provider and their structure cannot be modified on our end.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Vere-Grey
force-pushed
the
feat/suite-desktop-core/trading-buy-solana
branch
from
c288932 to
7832fb9
Compare
refactor of origin methods to support both btc and sol small cleanup
Vere-Grey
force-pushed
the
feat/suite-desktop-core/trading-buy-solana
branch
from
7832fb9 to
93b23f0
Compare
…tail the redirect rarely takes over 5s, which is the timeout of an expect
There was a problem hiding this comment.
Invity response copied from a live test run.
There was a problem hiding this comment.
Invity response copied from a live test run.
| export const createRedirectedTradeResponse = (url: string) => { | ||
| const redirectToDetail = `${url}coinmarket-redirect#detail/btc/normal/0/${buyTrade.trade.paymentId}`; | ||
| const modifiedTrade = cloneDeep(buyTrade); | ||
| export const createRedirectedTradeResponse = (params: { | ||
| symbol: NetworkSymbol; | ||
| tradeRequest: TradeRequest; | ||
| url: string; | ||
| }) => { | ||
| const redirectToDetail = `${params.url}coinmarket-redirect#detail/${params.symbol}/normal/0/${params.tradeRequest.trade.paymentId}`; | ||
| const modifiedTrade = cloneDeep(params.tradeRequest); |
There was a problem hiding this comment.
Adds parametrization of a tradeRequest and symbol so we can create a specific redirect for BTC and Solana.
The plan is also add one test for Etherum
| @@ -36,15 +41,19 @@ export const invityResponses = { | |||
| [invityEndpoint.exchangeWatch]: exchangeWatch, | |||
| [invityEndpoint.info]: info, | |||
| [invityEndpoint.buyList]: buyList, | |||
| [invityEndpoint.buyQuotes]: buyQuotes, | |||
| [invityEndpoint.buyQuotes]: buyQuotesBTC, | |||
There was a problem hiding this comment.
This object now contains default mocked response that will be used by all tests.
Test do on top of that add extra mocked responses. Like Solana changes quotes and trade to Solana ones
| async addUnusedHiddenWallet(passphrase: string) { | ||
| await this.addHiddenWallet(passphrase); |
There was a problem hiding this comment.
I am bit unsure about the naming.
The rest of the steps bellow await this.addHiddenWallet(passphrase); happens only if the wallet does not have anything on it, right?
| // This redirect is provided by Invity and normaly leads to provider's page. | ||
| // But our mocked response redirects us to transaction detail where our flow continues. | ||
| @step() | ||
| async mockInvityTrade(tradeRequest: TradeRequest, symbol: NetworkSymbol) { |
There was a problem hiding this comment.
moved out of mockInvity() so that in Solana we can mock solana and in BTC we can mock BTC
There was a problem hiding this comment.
Renaming, removing snapshots, adjusting to updated methods
| await expect(marketPage.transactionDetailStatus).toHaveText('Approved', { | ||
| timeout: 15_000, | ||
| }); |
There was a problem hiding this comment.
In rare cases the redirect takes over 5s. I was not able to findout why.
refactor of origin methods to support both btc and sol
small cleanup
Description
Expands Trading buy tests, this one cover different crypto = solana, and inputs the value by crypto instead of fiat
Related Issue
Resolve 16579
Screenshots:
https://app.currents.dev/instance/CiCJWZtnqCvSRD3M