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: Transaction Insights #12881

Merged
merged 69 commits into from
Dec 1, 2021
Merged

Conversation

alaahd
Copy link
Contributor

@alaahd alaahd commented Nov 29, 2021

Explanation:

This PR adds the Transactions Insights feature and supporting functionality. This will give users more information about what is happening when sending a transaction to a contract. The core of the functionality is displayed below:

transactioninsights.mp4

To achieve this, this PR uses https://github.com/trufflesuite/truffle/tree/develop/packages/decoder to decode transaction details which are retrieved from a codefi api

The above screen capture demonstrates the transaction decoding feature and the use of the new modals for viewing and editing account and nickname information.

In addition, the PR adds the transaction decoding to the transaction details modal that is opened when clicking list items in the transaction activity list. This addition also updates the styles/layouts of that modal.

Manual testing steps:

This can be done on the Kovan network.

  1. Create a transaction that requires interaction with a contract. Uniswap is a good example
  2. Inspect the "Data" tab to see the transaction details
  3. Click an address in the transaction details and edit the nickname
  4. Confirm the transaction
  5. Click the transaction in the activity list
  6. View the transaction information
  7. Click "Transaction Data"
  8. See the Transaction Data including the nickname you previously edited.

Screenshots and videos:

transaction-insights-activity-list.mp4

@alaahd alaahd requested a review from a team as a code owner November 29, 2021 16:29
@github-actions
Copy link
Contributor

github-actions bot commented Nov 29, 2021

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.

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

So there are few changes we need to this PR.

Here is how I would handle them, in order of priority:

  1. Rebase onto develop, so that the changes from this PR are not duplicated Created a Nickname popover #12632
  2. Refactor to use the current "Popover" pattern for showing modals, and then refactor to use react-redux hooks within functional components instead of "container" components. The "Popover" pattern is to import the respective popover component into the appropriate parent, pass the props from that parent directly to the component, and then conditionally render the component (and not to use redux for storing the data or indicating whether the popover should be shown). ui/components/app/qr-hardware-popover/qr-hardware-popover.js is an example
  3. It is probably best to have the confirm screen changes and the transaction details changes in separate PRs. This PR is big. Separate PRs will help code reviewers focus and it is possible that some of the code review can be parallelized if in separate PRs
  4. Move the logic in the useEffect of the transaction-decoding component to help methods in a util file
  5. Address the small comments

There is also some clean up that needs to happen of css, but that can be separate.

@gnidan
Copy link
Contributor

gnidan commented Nov 30, 2021

I have read the CLA Document and I hereby sign the CLA

@gnidan
Copy link
Contributor

gnidan commented Nov 30, 2021

Gabe raised the issue that the "copy transaction button" copies the text "[object Object]"

@alaahd
Copy link
Contributor Author

alaahd commented Nov 30, 2021

I have read the CLA Document and I hereby sign the CLA

@@ -0,0 +1,72 @@
import React, { useState } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

This file controls the display of the two nickname popovers:

Screenshot from 2021-12-01 09-40-45
Screenshot from 2021-12-01 09-40-41

@@ -0,0 +1,72 @@
import React, { useState } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

This component controls the display and behaviour of each icon+address in the decoded transaction information:
Screenshot from 2021-12-01 09-41-43

@@ -0,0 +1,38 @@
import React, { useContext } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for the "Copy raw transaction data" component in the hex tab:

Screenshot from 2021-12-01 09-42-44

@@ -0,0 +1,224 @@
import React, { useContext, useEffect, useState } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

This component is responsible for decoded transaction data, the primarily highlight of this feature/PR:

Screenshot from 2021-12-01 09-43-33

@@ -4,16 +4,17 @@ import copyToClipboard from 'copy-to-clipboard';
import { getBlockExplorerLink } from '@metamask/etherscan-link';
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes in this file are responsible for the layout changes in the transaction details modal:

Screenshot from 2021-12-01 09-45-09

@@ -0,0 +1,51 @@
import React, { useState, useRef, useEffect } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

This component handles the togglable dropdown ux and functionality shown below:

disclosure.mp4

@@ -8,6 +8,7 @@ import { shortenAddress } from '../../../helpers/utils/util';
import AccountMismatchWarning from '../account-mismatch-warning/account-mismatch-warning.component';
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes in this file account for the new behaviour of clicking the address shown at the top of the confirm screen (and in the transaction details modal):

sendertorecipientnew.mp4

jpuri
jpuri previously approved these changes Dec 1, 2021
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

👍 nice work

Please address feedbacks before merging.

segun
segun previously approved these changes Dec 1, 2021
className="address__name"
onClick={() => setShowNicknamePopovers(true)}
>
{addressOnly
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker but I'd prefer if this was assigned as a variable above the JSX.

Copy link
Contributor

Choose a reason for hiding this comment

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

done in 671e006


return (
<div className="copy-raw-data">
<Tooltip position="right" title={copied ? 'Copied!' : ''}>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use copiedExclamation localization here

Copy link
Contributor

Choose a reason for hiding this comment

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

}

&__icon {
padding-right: 6px;
Copy link
Contributor

Choose a reason for hiding this comment

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

padding-inline-end would be more friend to RTL.

Copy link
Contributor

Choose a reason for hiding this comment

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

font-size: 14px;
position: relative;
margin: 12px 0 4px;
padding-left: 22px;
Copy link
Contributor

Choose a reason for hiding this comment

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

padding-inline-start would be good here.

Copy link
Contributor

Choose a reason for hiding this comment

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

cursor: pointer;

& + .tx-insight-content {
padding-left: 14px;
Copy link
Contributor

Choose a reason for hiding this comment

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

padding-inline-start would be good here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@danjm danjm merged commit e056c88 into MetaMask:develop Dec 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2021
@gnidan gnidan deleted the feature/address-nicknames branch December 1, 2021 19:33
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants