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/incoming token transactions #1613

Merged
merged 28 commits into from
Jul 16, 2020

Conversation

estebanmino
Copy link
Contributor

@estebanmino estebanmino commented Jun 2, 2020

Description

https://trello.com/c/GbmznAVK/104-view-received-tokens

Needed to work MetaMask/core#247

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #???

@estebanmino estebanmino requested a review from a team as a code owner June 2, 2020 22:40
@estebanmino estebanmino marked this pull request as draft June 2, 2020 22:41
Copy link
Member

@rickycodes rickycodes left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me, just a few small notes.

app/util/transactions.js Outdated Show resolved Hide resolved
app/components/UI/TransactionElement/utils.js Outdated Show resolved Hide resolved
const ticker = getTicker(args.ticker);
let transactionType;
if (renderFullAddress(from) === selectedAddress) transactionType = TRANSACTION_TYPES.SENT_TOKEN;
else transactionType = TRANSACTION_TYPES.RECEIVED_TOKEN;
Copy link
Member

Choose a reason for hiding this comment

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

same as above ☝️

let transactionType;
if (renderFrom === selectedAddress) transactionType = TRANSACTION_TYPES.SENT_COLLECTIBLE;
else transactionType = TRANSACTION_TYPES.RECEIVED_COLLECTIBLE;

Copy link
Member

Choose a reason for hiding this comment

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

same as above ☝️

@estebanmino estebanmino marked this pull request as ready for review July 7, 2020 22:31
@estebanmino estebanmino added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jul 7, 2020
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Issue 1:

The txn history updates roughly every 15 seconds or so = http://recordit.co/iH31hzRKLQ
If you are viewing txn details and have the modal open, this action will automatically close the modal = http://recordit.co/HnTwgPoYj1

Issue 2:

I wanted to test canceling a txn, but found that I can't send any ETH on Rinkeby or Ropsten, this was the error I got on Rinkeby

image

Followed by this:
Screen Shot 2020-07-08 at 8 04 26 PM

Issue 3:

Padding is a bit off on the icon's on Android; seen here on a pixel 2 device

image

@ibrahimtaveras00 ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 9, 2020
@estebanmino estebanmino added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jul 10, 2020
@estebanmino
Copy link
Contributor Author

Issue 1 fixed with MetaMask/core@0db69d9
The other two on this PR

Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Issues 1 -3 have been resolved 👍

Issue 4:

Something weird with SAI is occurring where I sent SAI to an account, but it's appearing as received DAI; seen here = https://recordit.co/q5S2nZ0Hj5

image

@ibrahimtaveras00 ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed next release and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 15, 2020
@estebanmino estebanmino added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jul 15, 2020
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Fix looks good, QA Passed 👍

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 16, 2020
@estebanmino estebanmino merged commit bdf7641 into develop Jul 16, 2020
@estebanmino estebanmino deleted the feature/incoming-token-transactions branch July 16, 2020 17:07
rickycodes added a commit that referenced this pull request Jan 31, 2022
* wip

* filter tx by token

* tx icons

* fix type

* handle tx type

* delete log

* recived tx

* recived tx

* recived tx

* Apply suggestions from code review

Co-authored-by: ricky <ricky.miller@gmail.com>

* pass alethio key and package

* review suggestions

* lint

* lock packages

* update package

* stretch

* snaps

* controllers bump

* lock

* lock

* sai

Co-authored-by: ricky <ricky.miller@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants