First trading sell test for BTC #16889
Conversation
WalkthroughThe pull request updates multiple workflow configurations, test files, JSON fixtures, and types used in trading operations. In the workflow files for desktop and web e2e tests, steps related to uploading Playwright reports have been removed, and an environment variable for a passphrase has been added. Documentation for local test setup now includes an instruction to create a .env file. New JSON structures were introduced for buy and sell trade requests, trade responses, and cryptocurrency trading quotes. Changes in the test files adjust method signatures, endpoints, and payload expectations for both buying and selling operations. Additionally, component files have been enhanced with extra data-test identifiers to improve their testability. A new dependency on the dotenv package has been added, and types in the invity module were updated to clearly distinguish between standard and sell trade responses. Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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 (
|
refactors shared methods between sell and buy adds more verifications of request payloads simplifies the creation of Trade response
Vere-Grey
force-pushed
the
feat/suite-desktop-core/e2e-trading-sell
branch
from
bb04fcb to
f493c33
Compare
|
🚀 Expo preview is ready!
|
new passphrase wallet for trading tests few new locators new .env file for tests removes traces from CI
Vere-Grey
force-pushed
the
feat/suite-desktop-core/e2e-trading-sell
branch
from
f493c33 to
3b4a427
Compare
| @@ -25,6 +25,7 @@ Steps: | |||
| - ``export HOSTNAME=`hostname` `` | |||
| - `export DISPLAY=${HOSTNAME}:0` | |||
| 1. In terminal window, navigate to `trezor-user-env` repo root and run `./run.sh`. | |||
| 1. In workspace `packages/suite-desktop-core` create a `.env` file according to the `.example.env` | |||
There was a problem hiding this comment.
configuration for local runs
There was a problem hiding this comment.
generated by a run against live invity APIs
This applies for all json files in invity folder.
We use the requests json to validate suite send correct payloads to invity.
Atm we just not verifiing ids that change each time.
| sellWatch: `${invityUrl}/api/v3/sell/fiat/watch/*`, | ||
| }; | ||
|
|
||
| export const invityRequest = { |
There was a problem hiding this comment.
objects containing all the requests for convenient import and use
| 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); | ||
| modifiedTrade.trade.partnerData = redirectToDetail; | ||
| modifiedTrade.tradeForm.form.formAction = redirectToDetail; | ||
| export const createRedirectedTradeResponse = ( | ||
| tradeResponse: TradeResponse | SellTradeResponse, | ||
| tradeRequest: any, | ||
| ) => { | ||
| const modifiedResponse = cloneDeep(tradeResponse); | ||
| const newRedirect = tradeRequest.returnUrl; | ||
| modifiedResponse.trade.partnerData = newRedirect; | ||
| modifiedResponse.tradeForm.form.formAction = newRedirect; | ||
| modifiedResponse.trade.paymentId = tradeRequest.trade.paymentId; | ||
| modifiedResponse.trade.orderId = tradeRequest.trade.orderId; | ||
| if ('refundAddress' in modifiedResponse.trade && tradeRequest.refundAddress) { | ||
| modifiedResponse.trade.refundAddress = tradeRequest.refundAddress; | ||
| } | ||
|
|
||
| return modifiedResponse; | ||
| }; |
There was a problem hiding this comment.
I realized we don't need at all creating the URL. It is already in request. So we just need to return it in response.
I also realized I was returning wrong ids. So now we also adopt the correct ids from the request.
| @@ -33,7 +56,12 @@ type TradeForm = { | |||
| }; | |||
| }; | |||
|
|
|||
| export type TradeRequest = { | |||
| export type TradeResponse = { | |||
There was a problem hiding this comment.
A mistake from past PR
| @@ -41,7 +44,7 @@ const config: PlaywrightTestConfig = { | |||
| }, | |||
| reportSlowTests: null, | |||
| reporter: process.env.GITHUB_ACTION | |||
| ? [['list'], ['@currents/playwright'], ['html', { open: 'never' }]] | |||
There was a problem hiding this comment.
No need to generate PW reports Since we are not storing them on CI anymore.
| @@ -185,8 +185,8 @@ const test = base.extend<Fixtures>({ | |||
| const recoveryPage = new RecoveryActions(page); | |||
| await use(recoveryPage); | |||
| }, | |||
| marketPage: async ({ page, url }, use) => { | |||
| const marketPage = new MarketActions(page, url); | |||
| marketPage: async ({ page }, use) => { | |||
There was a problem hiding this comment.
we not need it thx to simplifing how we take return url instead of creating.
| @step() | ||
| async setYouPayFiatAmount( | ||
| async setYouPayAmount( | ||
| amount: string, | ||
| currency: FiatCurrencyCode = 'czk', | ||
| cryptoCurrency: string = 'bitcoin', | ||
| wantCrypto: boolean = false, | ||
| fiatCurrencyCode: FiatCurrencyCode = 'czk', | ||
| country: string = 'CZ', | ||
| ) { | ||
| // Warning: the field is initialized empty and gets default value after the first offer sync | ||
| await expect(this.youPayFiatInput).not.toHaveValue(''); | ||
| const inputField = wantCrypto ? this.youPayCryptoInput : this.youPayFiatInput; | ||
| await expect(inputField).not.toHaveValue(''); | ||
| await this.selectCountryOfResidence(country); | ||
| await this.selectFiatCurrency(currency); | ||
| const quotesPromise = this.page.waitForResponse(invityEndpoint.buyQuotes); | ||
| await this.youPayFiatInput.fill(amount); | ||
| await quotesPromise; | ||
| // Warning: Bug #16054, as a workaround we wait for offer sync after setting the amount | ||
| await this.waitForOffersSyncToFinish(); | ||
| } | ||
|
|
||
| @step() | ||
| async setYouPayCryptoAmount(amount: string, country: string = 'CZ') { | ||
| // Warning: the field is initialized empty and gets default value after the first offer sync | ||
| await expect(this.youPayCryptoInput).not.toHaveValue(''); | ||
| await this.selectCountryOfResidence(country); | ||
| await this.youPayCryptoInput.fill(amount); | ||
| // Warning: Bug #16054, as a workaround we wait for offer sync after setting the amount | ||
| await this.waitForOffersSyncToFinish(); | ||
| await this.selectFiatCurrency(fiatCurrencyCode); | ||
| const quotesRequestPromise = this.page.waitForRequest(invityEndpoint.buyQuotes); | ||
| const quotesResponsePromise = this.page.waitForResponse(invityEndpoint.buyQuotes); | ||
| await inputField.fill(amount); | ||
| await expect(quotesRequestPromise).toHavePayload({ | ||
| wantCrypto, | ||
| fiatCurrency: fiatCurrencyCode.toUpperCase(), | ||
| receiveCurrency: cryptoCurrency, | ||
| country, | ||
| fiatStringAmount: wantCrypto ? '2500' : amount, | ||
| ...(wantCrypto && { cryptoStringAmount: amount }), | ||
| }); | ||
| await quotesResponsePromise; | ||
| await this.waitForBuyOffersSync(); | ||
| } |
There was a problem hiding this comment.
merging setYouPayCryptoAmount and setYouPayFiatAmount to one method as they shared a lot of same steps.
There was a problem hiding this comment.
and enriching by request validation and await of the response
| @step() | ||
| async finishMockedTrade() { | ||
| const tradeRequestPromise = this.page.waitForRequest(invityEndpoint.buyTrade); | ||
| await this.confirmTradeButton.click(); | ||
| await expect(tradeRequestPromise).toHavePayload(expectedTradeRequestPayload, { | ||
| omit: ['returnUrl', 'trade.orderId', 'trade.paymentId'], | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
moved back to test level as different test verify different payloads. But now that I am looking at it. That could be just a one argument ...
| async mockInvityTrade(tradeResponse: TradeResponse | SellTradeResponse, endpointUrl: string) { | ||
| await this.page.route(endpointUrl, async (route, request) => { | ||
| const redirectedTradeResponse = createRedirectedTradeResponse( | ||
| tradeResponse, | ||
| request.postDataJSON(), | ||
| ); | ||
| await route.fulfill({ json: redirectedTradeResponse }); |
There was a problem hiding this comment.
createRedirectedTradeResponse is now taking the request payload as a parameter and use its returnUrl and ids in the response.
There was a problem hiding this comment.
also supports now sell trades by introducing endpointUrl
| await marketPage.finishMockedTrade(); | ||
| await expect(watchRequestPromise).toHavePayload(expectedWatchRequestPayload, { | ||
| omit: ['partnerData'], | ||
| await marketPage.changeBuyWatchResponseTo('SUBMITTED'); |
There was a problem hiding this comment.
I have changed the default of watches to be approved.
This test tho want to verify pending state of transaction -> so it sets the watch to status Submited
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
packages/suite-desktop-core/e2e/fixtures/invity/buy/requests/trade-request-solana.json (1)
1-24: Review: Validate Trade Object Structure and Data IntegrityThe JSON structure for the "trade" object is comprehensive and well-organized. All expected fields—including identifiers, currency details, amounts, rate, and additional metadata—are present and appear appropriately typed. A couple of considerations:
- Ensure the "exp" field (line 8) is consistently Base64 encoded as expected by downstream processes.
- Consider adding brief inline documentation or referencing external schema documentation to describe each field’s purpose, which will aid future maintainers.
packages/suite-desktop-core/e2e/fixtures/invity/sell/quotes-bitcoin.json (1)
72-90: Error Handling and Messaging: Fifth Sell Quote EntryThis entry simulates an error condition by including an "error" field alongside standard transaction details. The error message reads "Amount too low, minimum is 0.00065 0.00083102." To improve clarity, consider clarifying the two minimum values (likely fiat and crypto thresholds) with a clear delimiter or additional context. For example, you might change the message to indicate which value applies to fiat and which to crypto.
A possible diff to improve clarity:
-"error": "Amount too low, minimum is 0.00065 0.00083102.", +"error": "Amount too low, minimum is 0.00065 (fiat) / 0.00083102 (crypto).",packages/suite-desktop-core/e2e/fixtures/invity/index.ts (1)
75-90: Consider stronger typing for tradeRequest.
The function effectively modifies a copy of “tradeResponse,” but “tradeRequest: any” may hinder future maintenance. Stronger typing or a dedicated interface could improve clarity.-export const createRedirectedTradeResponse = ( - tradeResponse: TradeResponse | SellTradeResponse, - tradeRequest: any, -) => { +interface TradeRequestPayload { + returnUrl: string; + trade: { + paymentId: string; + orderId: string; + }; + refundAddress?: string; + // ...add more fields if needed +} + +export const createRedirectedTradeResponse = ( + tradeResponse: TradeResponse | SellTradeResponse, + tradeRequest: TradeRequestPayload, +) => {packages/suite-desktop-core/e2e/tests/trading/buy-ethereum.test.ts (1)
18-18: LGTM! Consider adding a constant for the currency name.The changes improve maintainability by using
invityEndpoint.buyTradeand updating the method signature. Consider extracting the hardcoded string 'ethereum' into a constant for better maintainability.+const CURRENCY = 'ethereum'; + test.describe('Trading - Buy Ethereum', { tag: ['@group=other', '@webOnly'] }, () => { test.beforeEach(async ({ page, marketPage, onboardingPage, dashboardPage }) => { await marketPage.mockInvity(); await marketPage.mockInvityTrade(buyTradeEthereum, invityEndpoint.buyTrade); // ... }); // ... - await marketPage.setYouPayAmount(fiatAmount, 'ethereum'); + await marketPage.setYouPayAmount(fiatAmount, CURRENCY);Also applies to: 38-38
packages/suite-desktop-core/e2e/tests/trading/sell-bitcoin.test.ts (2)
53-61: Consider adding timeout for quote request.The test waits for the quote request but doesn't specify a timeout. Consider adding a timeout to prevent test hangs.
- const quoteRequestPromise = page.waitForRequest(invityEndpoint.sellQuotes); + const quoteRequestPromise = page.waitForRequest(invityEndpoint.sellQuotes, { timeout: 10000 });
88-98: Add comment explaining the skipped sell button click.The code includes a comment about not clicking the sell button to avoid losing crypto, but it could be more explicit about this being a test limitation.
- // Don't click the sell button, we will lose crypto. + // Note: We intentionally skip clicking the sell button in tests to prevent actual cryptocurrency transactions. + // In a real scenario, the user would complete the transaction by clicking this button.packages/suite-desktop-core/e2e/tests/trading/buy-bitcoin.test.ts (1)
81-82: Consider extracting magic number.The watch period used in
page.clock.fastForwardcould be extracted into a named constant for better readability.+const WATCH_REFRESH_PERIOD_MS = 30000; // 30 seconds + test.describe('Trading - Buy BTC', { tag: ['@group=other', '@webOnly'] }, () => { // ... await test.step('Wait 30s for watch refresh and change of status to Approved', async () => { await marketPage.changeBuyWatchResponseTo('SUCCESS'); - await page.clock.fastForward(marketPage.watchPeriod); + await page.clock.fastForward(WATCH_REFRESH_PERIOD_MS);docs/tests/e2e-playwright-suite.md (1)
27-29: Refine instruction wording for clarity.
Consider varying the sentence beginnings in the "Run it locally" section (around line 28) to improve readability. For example, try rephrasing the step to "Please create a .env file according to the .example.env template" to avoid repetitive phrasing.🧰 Tools
🪛 LanguageTool
[style] ~28-~28: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r-envrepo root and run./run.sh. 1. In workspacepackages/suite-desktop-core`...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 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 (27)
.github/workflows/test-suite-desktop-e2e.yml(0 hunks).github/workflows/test-suite-web-e2e-pw.yml(1 hunks)docs/tests/e2e-playwright-suite.md(1 hunks)packages/suite-desktop-core/.example.env(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/buy/requests/trade-request-solana.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/buy/trade-bitcoin.json(2 hunks)packages/suite-desktop-core/e2e/fixtures/invity/index.ts(5 hunks)packages/suite-desktop-core/e2e/fixtures/invity/sell/list.json(2 hunks)packages/suite-desktop-core/e2e/fixtures/invity/sell/quotes-bitcoin.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/sell/requests/quotes-request.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/sell/requests/trade-request.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/sell/requests/watch-request.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/sell/trade-bitcoin.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/sell/watch.json(1 hunks)packages/suite-desktop-core/e2e/fixtures/invity/types.ts(2 hunks)packages/suite-desktop-core/e2e/playwright.config.ts(2 hunks)packages/suite-desktop-core/e2e/support/fixtures.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/devicePromptActions.ts(2 hunks)packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts(9 hunks)packages/suite-desktop-core/e2e/tests/trading/buy-bitcoin.test.ts(4 hunks)packages/suite-desktop-core/e2e/tests/trading/buy-ethereum.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/trading/buy-solana.test.ts(5 hunks)packages/suite-desktop-core/e2e/tests/trading/sell-bitcoin.test.ts(1 hunks)packages/suite-desktop-core/package.json(1 hunks)packages/suite/src/components/suite/FormattedCryptoAmount.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx(2 hunks)packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferSell/TradingOfferSellTransaction.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/test-suite-desktop-e2e.yml
✅ Files skipped from review due to trivial changes (4)
- packages/suite-desktop-core/e2e/fixtures/invity/sell/watch.json
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewOutputList/TransactionReviewOutputElement.tsx
- packages/suite-desktop-core/e2e/fixtures/invity/sell/requests/watch-request.json
- packages/suite-desktop-core/.example.env
🧰 Additional context used
🪛 LanguageTool
docs/tests/e2e-playwright-suite.md
[style] ~28-~28: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r-envrepo root and run./run.sh. 1. In workspace packages/suite-desktop-core`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Type Checking
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Unit Tests
- 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-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- 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=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: test
🔇 Additional comments (47)
packages/suite-desktop-core/e2e/fixtures/invity/buy/requests/trade-request-solana.json (1)
25-26: Review: Validate returnUrl ConfigurationThe "returnUrl" (lines 25-26) is set to a localhost URL, which is suitable for a test environment. Verify that this is properly replaced or parameterized in production builds via the new .env configuration. This will help avoid potential routing issues when transitioning from test to live environments.
packages/suite-desktop-core/e2e/fixtures/invity/sell/quotes-bitcoin.json (4)
1-18: Structure and Data Integrity: First Sell Quote EntryThis initial JSON object is well-formed and includes all the expected fields for a Bitcoin sell quote via the "moonpay-sell" exchange using a Credit Card. The numerical values (rate, min/max amounts) and string representations of amounts are clear. Verify that downstream consumers expect string values for fiat and crypto string amounts.
19-36: Validation of Time-Limited Quote: Second Sell Quote EntryThe second quote correctly introduces a "validUntil" field using an ISO8601 timestamp. It covers important values such as exchange details and transaction limits, ensuring that cases with time-limited quotes are adequately tested.
37-54: Consistency in Payment Methods: Third Sell Quote EntryThis entry represents a sell quote from "btcdirect-sell" using a SEPA payment method. Note that the "paymentMethodName" is provided in uppercase while "paymentMethod" is in lowercase. This variation is acceptable as long as the test consumers correctly interpret these fields. Double-check that this case difference is handled appropriately in your assertions.
55-71: Ensuring Correct Exchange and Payment Details: Fourth Sell Quote EntryThe fourth record mirrors the structure of the first entry but updates the payment method to SEPA for the "moonpay-sell" exchange. It is important to confirm that the tests differentiate scenarios based on both exchange and payment method, especially when the same exchange (moonpay-sell) appears with multiple payment methods.
packages/suite-desktop-core/e2e/fixtures/invity/index.ts (7)
7-9: Imports look consistent.
The new import statements for buy trade/watch payloads seem well-organized and follow existing naming conventions.
21-29: Good addition of sell-related payloads and types.
Bringing the sell workflow JSON files into this central fixture directory is consistent with the existing pattern for buy.
45-48: Verify correctness of newly introduced sell endpoints.
Please confirm that “/api/v3/sell/fiat/quotes,” “/api/v3/sell/fiat/trade,” and “/api/v3/sell/fiat/watch/*” match the actual backend routes.
50-56: Unified request object is well-structured.
Bundling buy and sell payloads in a single object under “invityRequest” simplifies usage and keeps the fixture consistent.
70-72: Sell responses mapped without issues.
Mapping the new sell endpoints to their fixture responses (e.g., “sellQuotesBTC,” “sellTradeBTC,” “sellWatch”) appears correct.
92-103: Helper function is concise and user-friendly.
The straightforward approach of filtering by provider name and throwing an error on mismatches is clear and dependable.
121-124: Additional exports finalized properly.
Exporting “sellQuotesBTC,” “sellTradeBTC,” and “sellWatch” is consistent with the rest of the file.packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts (12)
16-16: New type imports are in place.
Importing “SellTradeResponse” alongside “TradeResponse” ensures expanded coverage for sell scenarios.
50-50: Sell tab locator introduced.
Defining “sellTabButton” clarifies the UI element for switching to the sell interface.
92-92: Confirmation account locator.
Linking a dedicated locator for verifying the correct account is beneficial for extended test coverage.
114-114: Watch period recognized.
Storing watch interval as a string helps ensure consistent timing checks for potential polling logic.
115-115: Inline comment.
Labeling the code block for “Sell” is a simple, effective way to keep things organized.
116-116: Sell button locator correctly defined.
Having a separate sell button maintains clarity between buy flow and sell flow.
118-124: Constructor updates for sell functionality.
The added Locators for selling—such as “sellTabButton,” “sellTermsConfirmButton,” and “formSellButton”—are coherent. After correcting the test ID mismatch, the approach appears solid.Also applies to: 155-157, 162-162, 186-186
190-190: No functional change detected.
Line annotation indicates a modification here, but the logic remains the same. No concerns.
197-203: Sell offers synchronization mirrors buy flow.
Reusing a similar pattern for wait logic helps ensure consistent UI readiness checks.
241-265: Potential mismatch with “buyQuotes” endpoint.
When “wantCrypto” is true, the code still waits for “buyQuotes” requests. If this method is also intended for selling, consider a separate “sellQuotes” flow or a dynamic endpoint based on context.
284-289: Extends mock logic to handle sell.
Adapting “mockInvityTrade” to unify “TradeResponse” and “SellTradeResponse” is straightforward and fosters code reuse.
303-304: Method rename clarifies scope.
Changing “changeTransactionWatchResponseTo” into “changeBuyWatchResponseTo” better reflects that it only modifies the buy watch endpoint.packages/suite-desktop-core/e2e/fixtures/invity/types.ts (2)
28-49: SellTrade structure is well-differentiated from Trade.
The addition of “refundAddress,” “cryptoStringAmount,” and “amountInCrypto” effectively captures unique sell-specific fields.
59-67: Separate response types for buy and sell.
Splitting “TradeResponse” and “SellTradeResponse” ensures clarity and avoids overloading a single interface with extra optional fields.packages/suite-desktop-core/e2e/playwright.config.ts (2)
3-6: LGTM! Environment variables configuration added.The addition of dotenv configuration is well-placed at the beginning of the file to ensure environment variables are loaded before they're used.
47-47: LGTM! Removed HTML reports from CI.Removing HTML reports from CI aligns with the past review comment indicating they're no longer being stored.
packages/suite-desktop-core/e2e/support/pageActions/devicePromptActions.ts (2)
18-23: LGTM! Well-structured amount retrieval methods.The new methods follow consistent patterns and reuse existing test ID structures.
25-25: LGTM! Sell button integration.The sellButton property is properly initialized using a consistent test ID pattern.
Also applies to: 36-36
packages/suite-desktop-core/e2e/tests/trading/buy-solana.test.ts (3)
4-9: LGTM! Improved trade request mocking.The changes properly structure imports and use endpoint constants for mocking, improving maintainability.
Also applies to: 24-24
48-51: LGTM! Enhanced method clarity and parameters.The changes improve code clarity by:
- Using more specific method name
waitForBuyOffersSync- Adding explicit
isCryptoInputparameter
65-69: LGTM! Robust request verification.Good addition of payload verification with proper handling of dynamic fields through omission.
packages/suite/src/components/suite/FormattedCryptoAmount.tsx (1)
106-106: LGTM! Enhanced component testability.Added data-testid to Row component using a consistent pattern by extending the existing dataTest prop.
packages/suite-desktop-core/e2e/tests/trading/sell-bitcoin.test.ts (2)
31-35: LGTM! Environment variable check is well implemented.The check for the PASSPHRASE environment variable with a clear error message helps developers understand the test requirements.
71-74: Verify the redirection timing.The test uses a 15-second timeout for redirection. Verify if this is an appropriate duration based on production performance metrics.
packages/suite-desktop-core/e2e/tests/trading/buy-bitcoin.test.ts (1)
64-73: LGTM! Watch status handling is correctly implemented.The implementation aligns with the intended behavior of verifying the pending state of the transaction.
packages/suite/src/views/wallet/trading/common/TradingSelectedOffer/TradingOfferSell/TradingOfferSellTransaction.tsx (1)
70-70: LGTM! Test IDs follow consistent naming convention.The added data-testid attributes follow a clear naming pattern and improve component testability.
Also applies to: 78-79, 97-98, 109-109
packages/suite-desktop-core/e2e/support/fixtures.ts (1)
188-190: LGTM! Simplified fixture signature.The removal of the
urlparameter aligns with the simplified return URL handling approach.packages/suite-desktop-core/e2e/fixtures/invity/sell/requests/quotes-request.json (1)
1-9: Verify test transaction amounts.The test values appear reasonable, but please ensure:
- The crypto amount (0.00065 BTC) is appropriate for repeated test executions
- The selected country (CZ) and currency (EUR) combination is supported by Moonpay
✅ Verification successful
🌐 Web query:
What are Moonpay's supported countries and currencies for selling Bitcoin?💡 Result:
MoonPay supports selling Bitcoin (BTC) in **80+ countries** with direct cash payouts to Visa/Mastercard or bank accounts using local currencies[1][3]. Here's a detailed breakdown: ### Supported Countries MoonPay's Sell-to-Card feature for Bitcoin is available in: ``` Andorra | UAE | Argentina | Australia | Austria | Azerbaijan | Bulgaria | Bahrain | Bosnia & Herzegovina | Brazil | Switzerland | Chile | Costa Rica | Cyprus | Czech Republic | Germany | Denmark | Dominican Republic | Egypt | Spain | Estonia | Ethiopia | Finland | Georgia | Greece | Guatemala | Honduras | Croatia | Hungary | Indonesia | India | Ireland | Israel | Italy | Kazakhstan | Kenya | Kyrgyzstan | Kuwait | Lebanon | Saint Lucia | Liechtenstein | Lithuania | Luxembourg | El Salvador | Monaco | Moldova | Maldives | Mexico | North Macedonia*| Montenegro*| Mozambique*| Malawi*| Namibia*| Norway*| New Zealand*| Oman*| Peru*| Papua New Guinea*| Poland*| Portugal*| Paraguay*| Qatar*| Romania*| Saudi Arabia*| Singapore*| San Marino*| Serbia*| Slovakia*| Slovenia*| Sweden*| Thailand*| Turkmenistan*| Turkey*| Tanzania*| USA*| Uruguay*| Uzbekistan*| Vatican City State*| South Africa*| Zambia* ``` (*Partial list; full list includes 80+ nations)[1][3][4]. ### Supported Currencies - Payouts use **local fiat currencies** of supported countries (e.g., USD in the U.S., EUR in EU nations)[1][7]. - Major options include USD ($), EUR (€), GBP (£), AUD ($), CAD ($), CHF (Fr), and BRL (R$)[7]. - Bank transfers have lower fees (1%) compared to card payouts (up to 4.5%)[1][3]. ### Key Notes - Minimum BTC sell amount: $20[1]. - Funds arrive in 1–3 business days via bank transfers or instantly via Visa/Mastercard[1][3]. - Excludes unsupported regions like China, Russia, Japan[10]. Citations: - 1: https://www.moonpay.com/sell/btc - 2: https://www.moonpay.com/buy - 3: https://www.moonpay.com/sell - 4: https://www.moonpay.com/sell/bch - 5: https://play.google.com/store/apps/details?id=com.moonpay - 6: https://changelly.com/blog/what-is-moonpay-and-how-does-it-work/ - 7: https://support.moonpay.com/customers/docs/moonpays-supported-currencies - 8: https://proton.me/support/wallet-buy-btc-moonpay - 9: https://en.wikipedia.org/wiki/MoonPay - 10: https://support.moonpay.com/customers/docs/moonpays-unsupported-countries
Test Fixture Data Verification – Approved
- The crypto amount (0.00065 BTC) appears reasonable for testing. However, please ensure it does not inadvertently fall below the Moonpay minimum sell amount (approximately $20 based on current BTC valuations) during repeated test executions.
- The combination of "CZ" (Czech Republic) and "EUR" is supported by Moonpay's list of approved countries and currencies.
packages/suite-desktop-core/e2e/fixtures/invity/sell/requests/trade-request.json (1)
13-16: Verify transaction limits.Please confirm that the min/max limits match Moonpay's current requirements:
- minFiat: 20 EUR
- maxFiat: 30000 EUR
- minCrypto: 0.00006 BTC
- maxCrypto: 30000 BTC
packages/suite-desktop-core/e2e/fixtures/invity/buy/trade-bitcoin.json (2)
7-7: Consistent URL update for partnerData.
The new URL now provided in "partnerData" accurately reflects the integration with the payment processing mock; please double-check that the URL string is correctly formatted and matches the intended endpoint.
29-29: Synchronization of formAction with partnerData.
Setting "formAction" to the same URL as "partnerData" ensures that requests are directed consistently; verify that this meets your use-case requirements for the trading workflow.packages/suite-desktop-core/package.json (1)
61-61: Addition of dotenv dependency.
The inclusion of "dotenv": "^16.4.7" in devDependencies supports loading environment variables from a .env file, aligning with your updated local test setup instructions. Please confirm that this version is compatible with your toolchain and does not introduce conflicts.docs/tests/e2e-playwright-suite.md (1)
28-28: Enhanced local setup instructions.
The new step instructing users to create a .env file in the packages/suite-desktop-core workspace increases clarity for local test configuration. Ensure that the .env template (from .example.env) fully supports the passphrase and other required variables for running the tests.🧰 Tools
🪛 LanguageTool
[style] ~28-~28: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r-envrepo root and run./run.sh. 1. In workspacepackages/suite-desktop-core`...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
.github/workflows/test-suite-web-e2e-pw.yml (1)
158-158: Secure passphrase configuration for e2e tests.
Introducing the PASSPHRASE environment variable (sourced from secrets) is a secure method for passing credentials to your tests. Verify that the secret "E2E_TEST_PASSPHRASE" is correctly configured in your CI and that downstream processes can access it properly.packages/suite-desktop-core/e2e/fixtures/invity/sell/list.json (1)
376-392: Updated tradedCoins list for MoonPay Sell.
The "moonpay-sell" provider's "tradedCoins" list now includes several new coin identifiers (such as additional Ethereum contract addresses, binancecoin, tron, and specific Solana tokens) while reordering some entries. Please confirm that these changes align with the current trading functionality and that all token addresses are correct and supported by the backend system.
packages/suite-desktop-core/e2e/fixtures/invity/sell/requests/trade-request.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts (3)
153-157: Update test ID to match the pattern used in the code.The test ID for trade terms confirmation buttons should be consistent.
Apply this diff to maintain consistency:
- '@trading/buy/offers/trade-terms-confirm-button', + '@trading/buy/offers/buy-terms-confirm-button', - '@trading/sell/offers/trade-terms-confirm-button', + '@trading/sell/offers/sell-terms-confirm-button',
198-203: Consider extracting common wait logic.The
waitForSellOffersSyncmethod shares similar logic withwaitForBuyOffersSync. Consider extracting the common wait logic to reduce code duplication.+ private async waitForOffersSync(bestOfferButton: Locator) { + await expect(this.offerSpinner).toBeHidden({ timeout: 30000 }); + await expect(this.bestOfferAmount).not.toHaveText('0'); + await expect(bestOfferButton).toBeEnabled(); + } + @step() async waitForBuyOffersSync() { - await expect(this.offerSpinner).toBeHidden({ timeout: 30000 }); - await expect(this.bestOfferAmount).not.toHaveText('0 BTC'); - await expect(this.buyBestOfferButton).toBeEnabled(); + await this.waitForOffersSync(this.buyBestOfferButton); } @step() async waitForSellOffersSync() { - await expect(this.offerSpinner).toBeHidden({ timeout: 30000 }); - await expect(this.bestOfferAmount).not.toHaveText('0'); - await expect(this.formSellButton).toBeEnabled(); + await this.waitForOffersSync(this.formSellButton); }
241-265: Consider splitting the method based on responsibility.The
setYouPayAmountmethod handles both buy and sell scenarios, making it complex. Consider splitting it into separate methods for better maintainability.+ private async setAmount(inputField: Locator, amount: string) { + await expect(inputField).not.toHaveValue(''); + await inputField.fill(amount); + } + + private async waitForQuotes( + amount: string, + cryptoCurrency: string, + wantCrypto: boolean, + fiatCurrencyCode: FiatCurrencyCode, + country: string, + ) { + const quotesRequestPromise = this.page.waitForRequest(invityEndpoint.buyQuotes); + const quotesResponsePromise = this.page.waitForResponse(invityEndpoint.buyQuotes); + await expect(quotesRequestPromise).toHavePayload({ + wantCrypto, + fiatCurrency: fiatCurrencyCode.toUpperCase(), + receiveCurrency: cryptoCurrency, + country, + fiatStringAmount: wantCrypto ? '2500' : amount, + ...(wantCrypto && { cryptoStringAmount: amount }), + }); + await quotesResponsePromise; + } + @step() async setYouPayAmount( amount: string, cryptoCurrency: string = 'bitcoin', wantCrypto: boolean = false, fiatCurrencyCode: FiatCurrencyCode = 'czk', country: string = 'CZ', ) { const inputField = wantCrypto ? this.youPayCryptoInput : this.youPayFiatInput; - await expect(inputField).not.toHaveValue(''); await this.selectCountryOfResidence(country); await this.selectFiatCurrency(fiatCurrencyCode); - const quotesRequestPromise = this.page.waitForRequest(invityEndpoint.buyQuotes); - const quotesResponsePromise = this.page.waitForResponse(invityEndpoint.buyQuotes); - await inputField.fill(amount); - await expect(quotesRequestPromise).toHavePayload({ - wantCrypto, - fiatCurrency: fiatCurrencyCode.toUpperCase(), - receiveCurrency: cryptoCurrency, - country, - fiatStringAmount: wantCrypto ? '2500' : amount, - ...(wantCrypto && { cryptoStringAmount: amount }), - }); - await quotesResponsePromise; + await this.setAmount(inputField, amount); + await this.waitForQuotes(amount, cryptoCurrency, wantCrypto, fiatCurrencyCode, country); await this.waitForBuyOffersSync(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts(9 hunks)packages/suite-desktop-core/e2e/tests/trading/sell-bitcoin.test.ts(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TradingTermsModal.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TradingTermsModal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/suite-desktop-core/e2e/tests/trading/sell-bitcoin.test.ts
🧰 Additional context used
📓 Learnings (1)
packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts (1)
Learnt from: Vere-Grey
PR: trezor/trezor-suite#16889
File: packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts:89-89
Timestamp: 2025-02-07T15:01:54.986Z
Learning: Test IDs for trading terms confirmation buttons should match their respective actions: '@trading/sell/offers/sell-terms-confirm-button' for sell and '@trading/buy/offers/buy-terms-confirm-button' for buy.
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (4)
packages/suite-desktop-core/e2e/support/pageActions/marketActions.ts (4)
16-16: LGTM!The import of sell-specific types enhances type safety for the new selling functionality.
50-50: LGTM!The new locators for sell-related UI elements follow the established pattern and naming conventions.
Also applies to: 89-89, 92-92, 116-116
284-292: LGTM!The updated
mockInvityTrademethod now supports both buy and sell trade responses, making it more flexible.
304-308: LGTM!The method rename from
changeTransactionWatchResponseTotochangeBuyWatchResponseTobetter reflects its specific purpose.
Introduces first Trading sell test for BTC
Description
New test. Sell flow that ends by the sending. We cannot automate it further for the bitcoin.
Invity communication mocked. Introduces .env file for the tests and env variable from CI
Removes traces being attached to CI
Related Issue
Resolve 16579
Screenshots: