[FIX] Update E2E test token and related tests #1505
Conversation
…history item display for payments to C addres using lumens
| @@ -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(); | |||
There was a problem hiding this comment.
This isn't shown right now and also commented out so I assume we will need this again soon.
| })); | ||
| } else { | ||
| const { token, decimals } = balances[tokenKey] as TokenBalance; | ||
| try { |
There was a problem hiding this comment.
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.
| to = StrKey.encodeEd25519PublicKey( | ||
| args[1].address().accountId().ed25519(), | ||
| ); | ||
| from = addressToString(args[0].address()); |
There was a problem hiding this comment.
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.
…ing tx after sends in details
aristidesstaffieri
changed the title
|
|
||
| test("Send XLM payment to G address", async ({ page, extensionId }) => { | ||
| test.slow(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I just didn't observe that it added any longer of a timeout but no qualms with restoring, will add back.
There was a problem hiding this comment.
Cool - once we stub out external calls, we shouldn't need this anymore
| @@ -357,7 +357,9 @@ export const TransactionDetails = ({ goBack }: { goBack: () => void }) => { | |||
| url, | |||
| networkDetails, | |||
| ); | |||
| return; | |||
There was a problem hiding this comment.
does this do something the return on ln 388 does not?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
do we need to worry about this loading state for classic tx's, as well?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ok cool I think we're aligned here, 62cd1a6 is the version that I think handles this best.
What
Updates the E2E test token address for tests that use it.
Adds fetching of token details in history rows that are transfers in all cases.
Makes
getArgsForTokenInvocationwork with transfers to C addresses.Why
The address was stale after the testnet reset.
Transfers to C addresses uncovered a few edge cases in history.