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.

Sign up for GitHub

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

Jump to bottom

[FIX] Update E2E test token and related tests #1505

Merged
merged 10 commits into from
Oct 4, 2024
Merged

[FIX] Update E2E test token and related tests #1505

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3ea530f
adds new test token address after testnet reset
aristidesstaffieri Oct 3, 2024
7c1fc65
comments blockaid announcement, tweaks e2e details in tests
aristidesstaffieri Oct 3, 2024
1e9a2e9
fixes loading state for token transfer details after transfer, fixes …
aristidesstaffieri Oct 4, 2024
7e16e3a
updates test for history item change
aristidesstaffieri Oct 4, 2024
3713541
fix stale state instead by manually dismissing loading when not scann…
aristidesstaffieri Oct 4, 2024
8850176
restore login and fund on first payment test
aristidesstaffieri Oct 4, 2024
622aa4c
make e2e tests run in sequence
aristidesstaffieri Oct 4, 2024
8fed63c
restore slow timeout settings for payment tests
aristidesstaffieri Oct 4, 2024
fd2e8dc
avoids rescanning classic transactions after tx is sent in details
aristidesstaffieri Oct 4, 2024
62cd1a6
add explicit prop to scan tx in tx details
aristidesstaffieri Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions extension/e2e-tests/addAsset.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { test, expect, expectPageToHaveScreenshot } from "./test-fixtures";
import { loginToTestAccount, PASSWORD } from "./helpers/login";
import { TEST_TOKEN_ADDRESS } from "./helpers/test-token";

test("Adding unverified Soroban token", async ({ page, extensionId }) => {
test.slow();
Expand All @@ -15,13 +16,11 @@ test("Adding unverified Soroban token", async ({ page, extensionId }) => {
await expect(page.getByText("Your assets")).toBeVisible();
await page.getByText("Add an asset").click({ force: true });
await page.getByText("Add manually").click({ force: true });
await page
.getByTestId("search-token-input")
.fill("CAHX2LUNQ4YKNJTDEFW2LSFOXDAL4QI4736RV52ZUGCIRJK5U7MWQWW6");
await page.getByTestId("search-token-input").fill(TEST_TOKEN_ADDRESS);
await expect(page.getByTestId("asset-notification")).toHaveText(
"Not on your listsFreighter uses asset lists to check assets you interact with. You can define your own assets lists in Settings.",
);
await expect(page.getByTestId("ManageAssetCode")).toHaveText("E2E Token");
await expect(page.getByTestId("ManageAssetCode")).toHaveText("E2E");
await expect(page.getByTestId("ManageAssetRowButton")).toHaveText("Add");
await page.getByTestId("ManageAssetRowButton").click({ force: true });

Expand Down
4 changes: 2 additions & 2 deletions extension/e2e-tests/helpers/login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const loginAndFund = async ({ page, extensionId }) => {
});

await page.goto(`chrome-extension://${extensionId}/index.html#/account`);
await page.getByTestId("BlockaidAnnouncement__accept").click();
// await page.getByTestId("BlockaidAnnouncement__accept").click();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't shown right now and also commented out so I assume we will need this again soon.

await expect(page.getByTestId("network-selector-open")).toBeVisible({
timeout: 10000,
});
Expand Down Expand Up @@ -76,7 +76,7 @@ export const loginToTestAccount = async ({ page, extensionId }) => {
});

await page.goto(`chrome-extension://${extensionId}/index.html#/account`);
await page.getByTestId("BlockaidAnnouncement__accept").click();
// await page.getByTestId("BlockaidAnnouncement__accept").click();
await expect(page.getByTestId("network-selector-open")).toBeVisible({
timeout: 50000,
});
Expand Down
2 changes: 2 additions & 0 deletions extension/e2e-tests/helpers/test-token.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export const TEST_TOKEN_ADDRESS =
"CA5F3Q3KQWGG5J4U6OBCJEFVG4B5JVMHFRGQGMLNZXTEMO7YEO6UYMMD";
19 changes: 5 additions & 14 deletions extension/e2e-tests/sendPayment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { test, expect, expectPageToHaveScreenshot } from "./test-fixtures";
import { loginAndFund, loginToTestAccount, PASSWORD } from "./helpers/login";
import { TEST_TOKEN_ADDRESS } from "./helpers/test-token";

test("Send XLM payment to G address", async ({ page, extensionId }) => {
test.slow();
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a specific reason to drop this? if I recall, it just extends the wait time of a test. I believe I had added this in the event of slow responses from Horizon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just didn't observe that it added any longer of a timeout but no qualms with restoring, will add back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool - once we stub out external calls, we shouldn't need this anymore

await loginAndFund({ page, extensionId });
await page.getByTitle("Send Payment").click({ force: true });

Expand Down Expand Up @@ -72,15 +72,12 @@ test("Send XLM payment to G address", async ({ page, extensionId }) => {
});

test("Send XLM payment to C address", async ({ page, extensionId }) => {
test.slow();
await loginToTestAccount({ page, extensionId });

// send XLM to C address
await page.getByTitle("Send Payment").click({ force: true });
await expect(page.getByText("Send To")).toBeVisible();
await page
.getByTestId("send-to-input")
.fill("CAHX2LUNQ4YKNJTDEFW2LSFOXDAL4QI4736RV52ZUGCIRJK5U7MWQWW6");
await page.getByTestId("send-to-input").fill(TEST_TOKEN_ADDRESS);
await page.getByText("Continue").click({ force: true });

await expect(page.getByText("Send XLM")).toBeVisible();
Expand Down Expand Up @@ -172,9 +169,7 @@ test("Send SAC to C address", async ({ page, extensionId }) => {

// send SAC to C address
await page.getByTitle("Send Payment").click({ force: true });
await page
.getByTestId("send-to-input")
.fill("CAHX2LUNQ4YKNJTDEFW2LSFOXDAL4QI4736RV52ZUGCIRJK5U7MWQWW6");
await page.getByTestId("send-to-input").fill(TEST_TOKEN_ADDRESS);
await page.getByText("Continue").click({ force: true });

await page.getByTestId("send-amount-asset-select").click({ force: true });
Expand Down Expand Up @@ -227,17 +222,13 @@ test("Send token payment to C address", async ({ page, extensionId }) => {
await expect(page.getByText("Your assets")).toBeVisible();
await page.getByText("Add an asset").click({ force: true });
await page.getByText("Add manually").click({ force: true });
await page
.getByTestId("search-token-input")
.fill("CAHX2LUNQ4YKNJTDEFW2LSFOXDAL4QI4736RV52ZUGCIRJK5U7MWQWW6");
await page.getByTestId("search-token-input").fill(TEST_TOKEN_ADDRESS);
await page.getByTestId("ManageAssetRowButton").click({ force: true });
await page.getByTestId("add-asset").dispatchEvent("click");

// send E2E token to C address
await page.getByTitle("Send Payment").click({ force: true });
await page
.getByTestId("send-to-input")
.fill("CAHX2LUNQ4YKNJTDEFW2LSFOXDAL4QI4736RV52ZUGCIRJK5U7MWQWW6");
await page.getByTestId("send-to-input").fill(TEST_TOKEN_ADDRESS);
await page.getByText("Continue").click({ force: true });

await page.getByTestId("send-amount-asset-select").click({ force: true });
Expand Down
8 changes: 8 additions & 0 deletions extension/src/popup/components/__tests__/HistoryItem.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { render, waitFor, screen } from "@testing-library/react";
import { HistoryItem } from "popup/components/accountHistory/HistoryItem";
import { TESTNET_NETWORK_DETAILS } from "@shared/constants/stellar";
import * as sorobanHelpers from "popup/helpers/soroban";
import * as internalApi from "@shared/api/internal";

describe("HistoryItem", () => {
afterAll(() => {
Expand All @@ -20,6 +21,13 @@ describe("HistoryItem", () => {
amount: "100000000",
};
});
jest.spyOn(internalApi, "getTokenDetails").mockImplementation(() => {
return Promise.resolve({
decimals: 7,
name: "native",
symbol: "XLM",
});
});
it("renders SAC transfers as payments", async () => {
const props = {
accountBalances: {
Expand Down
47 changes: 33 additions & 14 deletions extension/src/popup/components/accountHistory/HistoryItem/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,17 +397,26 @@ export const HistoryItem = ({
setIconComponent(
<Icon.ArrowUp className="HistoryItem__icon--sent" />,
);
setIsLoading(true);

if (!tokenKey) {
// TODO: attempt to fetch token details, not stored
setRowText(operationString);
setTxDetails((_state) => ({
..._state,
headerTitle: translations("Transaction"),
operationText: operationString,
}));
} else {
const { token, decimals } = balances[tokenKey] as TokenBalance;
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that sending transfers from payments is better supported, it became more obvious that we can't rely on client side cached token details for this path because we more often don't have them(user has a classic balance in Freighter but send to C address through SAC transfer for example). This update just fetches token details for every row that is a transfer inline to avoid the inconsistencies.

const tokenDetailsResponse = await getTokenDetails({
contractId: attrs.contractId,
publicKey,
networkDetails,
});

if (!tokenDetailsResponse) {
setRowText(operationString);
setTxDetails((_state) => ({
..._state,
headerTitle: translations("Transaction"),
operationText: operationString,
}));
}

const { symbol, decimals } = tokenDetailsResponse!;
const code = symbol === "native" ? "XLM" : symbol;
const formattedTokenAmount = formatTokenAmount(
new BigNumber(attrs.amount),
decimals,
Expand All @@ -418,7 +427,7 @@ export const HistoryItem = ({
setBodyComponent(
<>
{paymentDifference}
{formattedTokenAmount} {token.code}
{formattedTokenAmount} {code}
</>,
);
setIconComponent(
Expand All @@ -428,7 +437,7 @@ export const HistoryItem = ({
<Icon.ArrowUp className="HistoryItem__icon--sent" />
),
);
setRowText(token.code);
setRowText(code);
setDateText(
(_dateText) =>
`${
Expand All @@ -440,9 +449,19 @@ export const HistoryItem = ({
isRecipient: _isRecipient,
headerTitle: `${
_isRecipient ? translations("Received") : translations("Sent")
} ${token.code}`,
operationText: `${paymentDifference}${formattedTokenAmount} ${token.code}`,
} ${code}`,
operationText: `${paymentDifference}${formattedTokenAmount} ${code}`,
}));
} catch (error) {
// falls back to only showing contract ID
setRowText(operationString);
setTxDetails((_state) => ({
..._state,
headerTitle: translations("Transaction"),
operationText: operationString,
}));
} finally {
setIsLoading(false);
}
} else {
setRowText(operationString);
Expand Down
4 changes: 3 additions & 1 deletion extension/src/popup/components/sendPayment/SendConfirm/TransactionDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ export const TransactionDetails = ({ goBack }: { goBack: () => void }) => {
const isPathPayment = useSelector(isPathPaymentSelector);
const { isMemoValidationEnabled } = useSelector(settingsSelector);
const isSwap = useIsSwap();
const { scanTx, data: scanResult, isLoading } = useScanTx();
const { scanTx, data: scanResult, isLoading, setLoading } = useScanTx();

const { t } = useTranslation();

Expand Down Expand Up @@ -357,7 +357,9 @@ export const TransactionDetails = ({ goBack }: { goBack: () => void }) => {
url,
networkDetails,
);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

does this do something the return on ln 388 does not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well I think we could safely drop it but in the case that we scanTx we don't need to manually setLoading(false) because it happens in the hook call. So the bug was that we relied on isLoading but didn't always set it correctly in the case here.
Dropping it should only result in calling that state setter once unnecessarily which is probably fine.

}
setLoading(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to worry about this loading state for classic tx's, as well?

Copy link
Contributor Author

@aristidesstaffieri aristidesstaffieri Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

no we don't because we seem to be calling that in every case here. I guess maybe I should ask about the intention of the condition to not scanSorobanTx if submission.submitStatus === ActionStatus.IDLE && transactionSimulation.preparedTransaction? I think that's actually correct since we don't really get anything from re-scanning it in tx details after the tx has been done. Maybe the miss here is that we should also be doing this in scanClassicTx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wait I wrote that! lol Yeah curious if you think that's right? I think we probably also want to skip the scan for classic in the same condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay took me a second to understand what was happening, but I get it now:

This conditional is just to make sure we've finished simming the tx, which is only necessary for Soroban tx's. So it makes sense that we only have the conditional on scanSorobanTx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this in fd2e8dc
I think it makes sense to not scan either tx type in tx details if we are rendering it after the tx has been sent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah maybe I misunderstand this conditional. I think really what we want here is to never scan the tx is we are rendering the details at confirmation so I will just add a prop that we can pass in explicitly at confirmation to make this less brittle.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if we go back to tx details from the Tx Success screen, we'll re-scan a classic tx, but we won't re-scan a soroban tx. And that's kind of a weird discrepancy. We probably shouldn't re-scan in either case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok cool I think we're aligned here, 62cd1a6 is the version that I think handles this best.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice - this lgtm!

};
const scanClassicTx = async () => {
const transaction = await getBuiltTx(
Expand Down
1 change: 1 addition & 0 deletions extension/src/popup/helpers/blockaid.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export const useScanTx = () => {
data,
error,
isLoading,
setLoading,
scanTx,
};
};
Expand Down
19 changes: 10 additions & 9 deletions extension/src/popup/helpers/soroban.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ export const parseTokenAmount = (value: string, decimals: number) => {
return wholeValue.shiftedBy(decimals).plus(fractionValue);
};

export const addressToString = (address: xdr.ScAddress) => {
if (address.switch().name === "scAddressTypeAccount") {
return StrKey.encodeEd25519PublicKey(address.accountId().ed25519());
}
return StrKey.encodeContract(address.contractId());
};

export const getArgsForTokenInvocation = (
fnName: string,
args: xdr.ScVal[],
Expand All @@ -121,18 +128,12 @@ export const getArgsForTokenInvocation = (

switch (fnName) {
case SorobanTokenInterface.transfer:
from = StrKey.encodeEd25519PublicKey(
args[0].address().accountId().ed25519(),
);
to = StrKey.encodeEd25519PublicKey(
args[1].address().accountId().ed25519(),
);
from = addressToString(args[0].address());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we support transfers to C addresses, I ran into this where from and to are actually not always accountId and can also be contractId.

to = addressToString(args[1].address());
amount = scValToNative(args[2]);
break;
case SorobanTokenInterface.mint:
to = StrKey.encodeEd25519PublicKey(
args[0].address().accountId().ed25519(),
);
to = addressToString(args[0].address());
amount = scValToNative(args[1]);
break;
default:
Expand Down
1 change: 1 addition & 0 deletions extension/src/popup/views/__tests__/SendPayment.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ jest.spyOn(BlockaidHelpers, "useScanTx").mockImplementation(() => {
return {
scanTx: () => Promise.resolve(null),
isLoading: false,
setLoading: () => {},
data: null,
error: null,
};
Expand Down
1 change: 1 addition & 0 deletions extension/src/popup/views/__tests__/Swap.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ jest.spyOn(BlockaidHelpers, "useScanTx").mockImplementation(() => {
return {
scanTx: () => Promise.resolve(null),
isLoading: false,
setLoading: () => {},
data: null,
error: null,
};
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"start:unpacked": "yarn workspace extension start:unpacked-extension",
"install-if-package-changed": "git diff-tree -r --name-only --no-commit-id ORIG_HEAD HEAD | grep --quiet yarn.lock && yarn setup || exit 0",
"test:ci": "jest --ci",
"test:e2e": "yarn workspace extension test:e2e",
"test:e2e": "yarn workspace extension test:e2e --workers=1",
"test": "jest -o --watch",
"prepare": "husky install"
},
Expand Down