-
Notifications
You must be signed in to change notification settings - Fork 5k
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
feat: SOL-80 transaction details #29323
Conversation
…github.com:MetaMask/metamask-extension into SOL-46-adds-the-multichain-transactions-controller
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
…o SOL-80-transaction-details
Builds ready [d6d7a16]
Page Load Metrics (1613 ± 64 ms)
Bundle size diffs
|
Builds ready [3403374]
Page Load Metrics (1800 ± 93 ms)
Bundle size diffs
|
Builds ready [6996ee1]
Page Load Metrics (1539 ± 44 ms)
Bundle size diffs
|
ui/components/app/transaction-list/transaction-list.component.js
Outdated
Show resolved
Hide resolved
ui/components/app/transaction-list/transaction-list.component.js
Outdated
Show resolved
Hide resolved
ui/components/app/multichain-transaction-details-modal/multichain-transaction-details-modal.tsx
Outdated
Show resolved
Hide resolved
…mask-extension into SOL-80-transaction-details
Builds ready [684e9b6]
Page Load Metrics (1819 ± 62 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (I will update the URL handling in another PR, as mentionned in some comments).
I've only tested the storybook part, but the video shows everything and the storybook displays properly.
* @param chainId - Network chain ID | ||
* @returns Full URL to transaction in block explorer, or empty string if no explorer URL | ||
*/ | ||
export const getTransactionUrl = (txId: string, chainId: string): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed internally, I think this kind of logic will not scale well in the future.
I have prepared another PR that will use format-strings instead to build block explorer URLs:
I'll make the changes to support transactions once we have merged this PR!
* @param chainId - Network chain ID | ||
* @returns Full URL to address in block explorer, or empty string if no explorer URL | ||
*/ | ||
export const getAddressUrl = (address: string, chainId: string): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
|
||
// It's typical for Solana timestamps to use seconds, while JS Dates and most EVM chains use milliseconds. | ||
// Hence we needed to use the conversion `timestamp < 1e12 ? timestamp * 1000 : timestamp` for it to work. | ||
const timestampMs = timestamp < 1e12 ? timestamp * 1000 : timestamp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since timestamps must be UNIX timestamp, they should be using "seconds" already, so I think we should just use timestamp * 1000
.
However, since only the Solana Snap implements this for now, I'll keep it as is and we will revisit this later once we have stabilized everything!
it('shows transaction ID in shortened format', () => { | ||
renderComponent(); | ||
const txId = mockTransaction.id; | ||
const shortenedTxId = screen.getByText(shortenAddress(txId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: But this should have been shortenTransactionId
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add in an other PR
Description
This PR adds the new transaction details modal for the non-evm networks, BTC and SOL.
Screen.Recording.2024-12-17.at.12.27.40.mov
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/SOL-80
Manual testing steps
Testing this is a bit extensive, but if you still want to give it a go these are the steps:
yarn
shared/lib/accounts/solana-wallet-snap.ts
with:export const SOLANA_WALLET_SNAP_ID: SnapId = 'local:http://localhost:8080' as SnapId;
node_modules/@metamask/multichain-transactions-controller/dist/MultichainTransactionsController.mjs
andnode_modules/@metamask/multichain-transactions-controller/dist/MultichainTransactionsController.cjs
with:yarn start:flask
yarn
yarn start
Screenshots/Recordings
Before
Didn't exist.
After
Pre-merge author checklist
Pre-merge reviewer checklist