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

Add Transaction Details to the Transaction List view #5182

Merged
merged 14 commits into from
Sep 13, 2018
Merged

Conversation

alextsg
Copy link
Contributor

@alextsg alextsg commented Aug 31, 2018

image

@alextsg alextsg requested review from danjm and whymarrh as code owners August 31, 2018 20:23
@alextsg alextsg force-pushed the tx-activity branch 2 times, most recently from 7fdc2a3 to debffb0 Compare September 1, 2018 04:32
@metamaskbot
Copy link
Collaborator

Builds ready [debffb0]: mascara, chrome, firefox, edge, opera

@@ -0,0 +1,26 @@
import React 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.

I just noticed that we have been using different naming conventions for test files. I've been doing name-component.test.js for the test file corresponding to name.component.js... I think I like yours better.

numberOfDecimals: 6,
})
export function getEthConversionFromWeiHex ({ value, conversionRate, numberOfDecimals = 6 }) {
const denominations = [ETH, GWEI, WEI]
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am reading this right, it is to ensure that the activity log shows the value associated with each activity in the largest possible denomination. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, or another way of putting it, if the value is too small for a particular denomination (and the result is 0 ETH, for example), it goes to a lower denomination and so on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for the "transaction created" row, the value should always be shown in ETH. When the user creates the transaction, they will be creating it in ETH, so showing in GWEI or WEI is confusing.

screenshot from 2018-09-06 15-14-45

@@ -103,3 +105,13 @@ export async function isSmartContractAddress (address) {
const code = await global.eth.getCode(address)
return code && code !== '0x'
}

export function addHex (...args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call this sumHexes so that it is clear it can accept any number of arguments?

}

default: {
events.push(eventCreator(value, timestamp))
Copy link
Contributor

Choose a reason for hiding this comment

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

events.push(eventCreator(value, timestamp))

The eventCreator(value, timestamp) call needs some default parameter for the eventKey

@danjm
Copy link
Contributor

danjm commented Sep 6, 2018

Styling issue in contract deployment 'sender-to-recipient'

screenshot from 2018-09-06 15-37-05

@danjm
Copy link
Contributor

danjm commented Sep 6, 2018

I wonder if contract deployments should have a different creation message than 'Transaction created with a value of 0 WEI.'? I'm not positive about this, but I think it is worth considering.

@danjm
Copy link
Contributor

danjm commented Sep 6, 2018

Token transactions should probably show token value for "Amount" in transaction breakdown and for the 'Transaction created' row in the activity log.

screenshot from 2018-09-06 15-41-57

@danjm
Copy link
Contributor

danjm commented Sep 6, 2018

The recipient address in the 'sender-to-recipient' for token transactions should probably be the address of the account to which the tokens were transferred, instead of the contract transaction.

@danjm
Copy link
Contributor

danjm commented Sep 6, 2018

The previous two comments also apply to Approvals.

@metamaskbot
Copy link
Collaborator

Builds ready [3df31db]: mascara, chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [f1a309e]: mascara, chrome, firefox, edge, opera

@alextsg alextsg merged commit 16d6cd5 into develop Sep 13, 2018
@alextsg alextsg deleted the tx-activity branch September 13, 2018 03:08
@tmashuang tmashuang mentioned this pull request Sep 17, 2018
@tmashuang tmashuang restored the tx-activity branch September 25, 2018 15:26
@whymarrh whymarrh deleted the tx-activity branch November 19, 2019 15:00
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.

3 participants