Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: E2E real wallet data using esbuild #193

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gbarkhatov
Copy link
Contributor

Added real wallet E2E tests

  1. Wallets are generated via real mnemonic. Both mainnet/signet and taproot/native segwit
  2. Since window.btcwallet is something we need to inject, and we do want our transactions to be real, we have to build our wallets with all the necessary bitcoin libs. We do it using esbuild
  3. The only mocking part is the initial UTXOs and POST requests. I.e. under the hood real BTC signatures are happening
  4. By default, Playwright will use the .env.local that you use. If you want to use a specific mainnet and signet env variables, you need to create .env.mainnet and .env.signet.
  5. You can use whatever wallets you want in your tests, but some of them require the specific ones. For example, in case of balance and address test, we can connect both signet and mainnet wallets and check the stuff. But anything with the signing involved will require specific env variables. You cannot use mainnet env variables with global parameters expecting X mainnet BTC Tip height on signet wallet. Since signet tip height is going to be differentSostaking, unbonding, withdrawaluse a set of mapping and test required network wallets only. That's because in our app we usesrc/app/context/mempool/BtcHeightProvider.tsxwhich will giveXwhile wallet mempool endpoint might giveYdepending on theenv` variables.

Besides reviewing the code I expect you run the instructions in Readme.md:

  • npm i <- install the dependencies
  • npx playwright install <- this is important
  • npm run test:e2e <- for your .env.local or
  • npm run test:e2e:full <- for your .env.mainnet and .env.signet

Added an E2E section in Readme.md

Video: https://youtu.be/jppgJa9L6gM

Closes:

Partly closes, so the PR is not even bigger:

@gbarkhatov gbarkhatov marked this pull request as ready for review October 7, 2024 12:54
@gbarkhatov
Copy link
Contributor Author

@totraev @jrwbabylonlab @jeremy-babylonlabs network is mocked, tests are working with wifi turned off

### Environment Configuration
The E2E tests can be run using either the .env.local file or the specific environment files .env.mainnet and .env.signet. These files should contain the following environment variables:

- `NEXT_PUBLIC_NETWORK`: Specifies the BTC network environment (mainnet or signet).
Copy link
Collaborator

@jrwbabylonlab jrwbabylonlab Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the E2E tests are using fully mocked data, do we still need all the environment setups across different env files? or maybe we can just create a single .env.test under the test directory for running the e2e test?

type AddressType = "taproot" | "nativeSegwit";

// Determine the network from environment variable or default to 'mainnet'
const network: Network =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not use existing network from the network.config.ts?

import { taprootSignetBalance } from "./mock/signet/wallet/taproot/utxos";

type Network = "mainnet" | "signet";
type AddressType = "taproot" | "nativeSegwit";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated with the types defined in WalletType

});
// Iterate over each address type
for (const type of addressTypes) {
test.describe(`${type} address on ${network}`, () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice the network is controlled by the env passed in.
Why not we make a testcase set here and let the for loop to cover both networks instead of passing it via env?
So something like this:

const testCases: [AddressType, Network][] = [
  ["taproot", "mainnet"],
  ["taproot", "signet"],
  ["nativeSegwit", "mainnet"],
  ["nativeSegwit", "signet"],
];

then we just do a testCase.forEach to loop through
What you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrwbabylonlab why we need test against multiple networks if everything is mocked?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically, it should be the same. The different network only matters if our code is performing BTC-specific operations. I’m not sure if our simple-staking implementation includes that (it shouldn’t), but it wouldn’t hurt to run it on multiple networks just to be safe.

"**/tip/height**",
200,
isOnMainnet ? mainnetTipHeight : signetTipHeight,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can do this to simplify a bit

const mocks = [
  { path: "**/healthcheck**", data: healthCheck },
  { path: "**/v1/finality-providers**", data: finalityProviders },
  { path: "**/fees/recommended**", data: network === "mainnet" ? mainnetFees : signetFees },
  ... and others
];

mocks.forEach(async ({ path, data }) => {
  await interceptRequest(page, path, 200, data);
});


// Setup wallet connection before each test
test.beforeEach(async ({ page }) => {
await setupWalletConnection(page, network, type);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s no need to use two separate test.beforeEach calls. It's the same as a single beforeEach

@@ -0,0 +1 @@
export type WalletType = "taproot" | "nativeSegwit";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name WalletType is a bit misleading tbh as this is actually address types that we currently support.

const publicKeyNoCoord =
getPublicKeyNoCoord(publicKeyHex).toString("hex");

// Verify the local storage has the correct staking details
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of verifying if localStorage have the item, would it beteer to just inspect the delegation list html if it contain the newly created pending delegation?
IMO, the e2e test is a blockbox test that it doesn't need to know we are using localStorage. The only thing it care is click button, expect something to appear in UI

updatedTX,
);
await setupWalletConnection(page, network, type);
await expect(page.getByText("Unbonded", { exact: true })).toBeVisible();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we locate to the exact tx id? otherwise if we expand the test to include multuple delegations, it won't work nicely

Copy link
Collaborator

@jrwbabylonlab jrwbabylonlab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with some comments, and below two actions:

  1. could we add e2e into the pipeline?
  2. could you set up the playwright config file that can help run the tests a few times? maybe 2-5 times is enough

@gbarkhatov
Copy link
Contributor Author

gbarkhatov commented Oct 9, 2024

could you set up the playwright config file that can help run the tests a few times? maybe 2-5 times is enough

@jrwbabylonlab This might be slow 😅

Copy link
Contributor

@totraev totraev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general:

  1. I think we should follow community standards and use fixtures over helper functions.
  2. If we don't plan to use a separate repo for test wallet we need optimise current implementation. At the moment we recompile and rebuild wallet before each test(!):
    test.beforeEach > setupWalletConnection > injectBTCWallet > build
  3. Instead of using interceptRequest I'd suggest to run mock server and keep all mock there. It'll significantly simplify things

@@ -115,6 +115,8 @@ web_modules/
.env.test.local
.env.production.local
.env.local
.env.mainnet
.env.signet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm run test:e2e to run the test using the network in .env.local

Screenshot 2024-10-09 at 12 50 29

When I run npm run test:e2e I see this. Is that expected behaviour?

Comment on lines +32 to +53
const expectedAddresses: Record<AddressType, Record<Network, string>> = {
taproot: {
mainnet: TAPROOT_MAINNET_ADDRESS,
signet: TAPROOT_SIGNET_ADDRESS,
},
nativeSegwit: {
mainnet: NATIVE_SEGWIT_MAINNET_ADDRESS,
signet: NATIVE_SEGWIT_SIGNET_ADDRESS,
},
};

// Mappings for expected balances based on network and address type
const expectedBalances: Record<AddressType, Record<Network, number>> = {
taproot: {
mainnet: taprootMainnetBalance,
signet: taprootSignetBalance,
},
nativeSegwit: {
mainnet: nativeSegwitMainnetBalance,
signet: nativeSegwitSignetBalance,
},
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move this logic together with current network to constants folder and return only taproot/nativeSegwit values depends on current network value:

const addresses = {
  mainnet: {
    taproot: 'address',
    nativeSegwit: 'address',
  },
  signet: {
    taproot: 'address',
    nativeSegwit: 'address',
  }
}[network];

});
// Iterate over each address type
for (const type of addressTypes) {
test.describe(`${type} address on ${network}`, () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrwbabylonlab why we need test against multiple networks if everything is mocked?

Comment on lines +82 to +93
test(`should verify the ${type} address and balance on ${network}`, async ({
page,
}) => {
const expectedAddress = expectedAddresses[type][network];
const expectedBalance = expectedBalances[type][network];

// Perform address check
await checkAddress(page, expectedAddress);

// Perform balance check
await checkBalance(page, expectedBalance);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test is pretty small, no need in additional wrapper. Weird to see test without expect expression

Suggested change
test(`should verify the ${type} address and balance on ${network}`, async ({
page,
}) => {
const expectedAddress = expectedAddresses[type][network];
const expectedBalance = expectedBalances[type][network];
// Perform address check
await checkAddress(page, expectedAddress);
// Perform balance check
await checkBalance(page, expectedBalance);
});
test(`should verify the ${type} address and balance on ${network}`, async ({
page,
}) => {
const expectedAddress = expectedAddresses[type][network];
const expectedBalance = expectedBalances[type][network];
// Perform address check
const address = await page.getByTestId("address").textContent();
expect(address).toBe(trim(expectedAddress));
// Perform balance check
const balanceText = await page.getByTestId("balance").textContent();
const balance = maxDecimals(extractNumericBalance(balanceText), 8);
expect(balance).toBe(satoshiToBtc(expectedBalance));
});

try {
const walletPath = path.resolve(__dirname, "btcWallet.ts");

const result = await build({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we should move it to a separate repo

@gbarkhatov
Copy link
Contributor Author

Suggestion - finalize after the Tomo dual wallet onboarded

@gbarkhatov gbarkhatov mentioned this pull request Oct 16, 2024
@gbarkhatov
Copy link
Contributor Author

One thing we discussed with @jrwbabylonlab - we can skip the tests because of the UI, wallet, processes (staking, unbonding, withdrawal) and enable them as soon as the needed parts are working and settled. I.e. when we add working wallet solution we can enable wallet e2e tests

@gbarkhatov
Copy link
Contributor Author

Converting this to draft until new changes are implemented related to Tomo wallet injectable and selectors

@gbarkhatov gbarkhatov marked this pull request as draft October 25, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants