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

481-bug-link-to-flowscan-broken #482

Merged
merged 3 commits into from
Feb 11, 2025
Merged

Conversation

tombeckenham
Copy link
Collaborator

Related Issue

Closes #481

Summary of Changes

No longer changes hash in transactions. Instead checks evm id and cadence id lists for matching transactions. Frontend tests now use that if they exist to build the ids

Need Regression Testing

Check flowscan for all transaction types

  • Yes
  • No

Risk Assessment

  • Low
  • Medium
  • High

Copy link
Member

@Kay-Zee Kay-Zee left a comment

Choose a reason for hiding this comment

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

Doesn't seem to link correctly when it's an EVM tx. I get sent to the EVM flow scan, but it's using the Cadence tx ID

EDIT: To add a bit more detail, this only happens with the brand new row in the activities column after i send an EVM -> EVM transaction.

Printing out the tx in the ListItemButton's onclick shows the following

Screenshot 2025-02-07 at 4 47 00 PM

Which as you can see, has the cadenceTxId as the hash. which means it will get used as the tx hash when linking out, and since i'm on the "EVM" side of the app, the flowscan URL will be evm.flowscan.

I tested that if i "reload" the extension and activities page, then i get sent to the correct page.

Changing the onClick to:

onClick={() => {
  {
    const txHash = (tx.evmTxIds && tx.evmTxIds.length) === 1 ? tx.evmTxIds[0] : tx.hash;
    const url =
      monitor === 'flowscan'
        ? `${flowscanURL}/tx/${txHash}`
        : `${viewSourceURL}/${txHash}`;
    window.open(url);
  }
}}

does solve the issue for me, but not sure if that's how you want to fix as this makes some assumptions about the tx.evmTxIds array.

Copy link
Collaborator Author

@tombeckenham tombeckenham left a comment

Choose a reason for hiding this comment

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

I see what you mean. It's possible to have multiple EVM transactions, but think your solution is cleanest

Copy link
Collaborator Author

@tombeckenham tombeckenham left a comment

Choose a reason for hiding this comment

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

I see what you mean. It's possible to have multiple EVM transactions, but think your solution is cleanest

The alternative would be to have no flowscan url

@tombeckenham tombeckenham requested review from zzggo and Kay-Zee and removed request for Kay-Zee, bthaile and nialexsan February 10, 2025 05:56
@tombeckenham tombeckenham merged commit 879901a into dev Feb 11, 2025
3 checks passed
@tombeckenham tombeckenham deleted the 481-bug-link-to-flowscan-broken branch February 11, 2025 01:39
@tombeckenham tombeckenham linked an issue Feb 26, 2025 that may be closed by this pull request
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.

[BUG] Link to flowscan broken
3 participants