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

Address book name save fix #6945

Merged
merged 5 commits into from
Aug 2, 2019
Merged

Address book name save fix #6945

merged 5 commits into from
Aug 2, 2019

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Aug 2, 2019

Should fix issues with saving contacts.

Adds e2e tests to cover the following flow:

  • add contact during send and successfully send
  • send another transaction by clicking previously added contact in the list

@danjm danjm requested review from Gudahtt and whymarrh as code owners August 2, 2019 00:59
@danjm danjm force-pushed the address-book-name-save-fix branch from 3de78eb to 2fe540a Compare August 2, 2019 20:30
Gudahtt
Gudahtt previously approved these changes Aug 2, 2019
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -51,6 +50,7 @@ concurrently --kill-others \
'npm run sendwithprivatedapp' \
'sleep 5 && mocha test/e2e/incremental-security.spec'

export GANACHE_ARGS="$GANACHE_ARGS --deterministic --account=0x53CB0AB5226EEBF4D872113D98332C1555DC304443BEE1CF759D15798D3C55A9,25000000000000000000"
Copy link
Member

@Gudahtt Gudahtt Aug 2, 2019

Choose a reason for hiding this comment

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

The $GANACHE_ARGS here includes all of the previous exported values (e.g. it'll have each of the --account flags listed one after another, along with the 3 instances of the --deterministic flag).

I'm guessing this isn't what we want. This was broken already, but I'm mentioning in case it relates to the test failure.

Copy link
Member

Choose a reason for hiding this comment

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

This could most easily be fixed by renaming the first case as BASE_GANACHE_ARGS or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that could be another PR

Copy link
Member

Choose a reason for hiding this comment

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

This has been addressed in #6970

@metamaskbot
Copy link
Collaborator

Builds ready [1d00f6e]: chrome, firefox, edge, opera

@danjm danjm merged commit 165f44d into develop Aug 2, 2019
@whymarrh whymarrh deleted the address-book-name-save-fix branch August 6, 2019 14:16
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