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.

Sign up for GitHub

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

Jump to bottom

Validate txParams in TransactionStateManager.addTx #6713

Merged
merged 3 commits into from
Jun 28, 2019
Merged

Validate txParams in TransactionStateManager.addTx #6713

merged 3 commits into from
Jun 28, 2019

Conversation

akshitkrnagpal
Copy link
Contributor

Fixes #6707.

@whymarrh
Copy link
Contributor

@akshitkrnagpal thanks for this, would you mind rebase this on the latest develop and fixing the conflicts?

@akshitkrnagpal
Copy link
Contributor Author

@whymarrh
Hey
I have updated the PR. It would be great if you can review it. 😄

frankiebee
frankiebee suggested changes Jun 21, 2019
View reviewed changes
Copy link
Contributor

@frankiebee frankiebee left a comment

Choose a reason for hiding this comment

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

do you mind writing tests for this?

@akshitkrnagpal
Copy link
Contributor Author

akshitkrnagpal commented Jun 21, 2019 •
edited
Loading

@frankiebee
Sure.

Would these tests cover the needs?
When txParams are invalid -

  • addTx throws error and the tx is not added in the list.
  • updatedTx throws error and tx is not updated in the list.

For which invalid values should I test this for? txParams can be invalid if any of the members is invalid and invalid values for each member can be anything other than of type string (except chainId which can also be a number).

@frankiebee
Copy link
Contributor

i would test for gas passed as an object sense that seems to be the issue in reference but yes i would test the keys in txParams

Would these tests cover the needs?
When txParams are invalid -

addTx throws error and the tx is not added in the list.
updatedTx throws error and tx is not updated in the list.

this is good yes

@akshitkrnagpal
Copy link
Contributor Author

@frankiebee
Hey
I have added tests. Can you please review? 😄

@akshitkrnagpal
Copy link
Contributor Author

@Gudahtt Thanks for approving.

@frankiebee Can you take a look so this can be merged? 😄

frankiebee
frankiebee approved these changes Jun 28, 2019
View reviewed changes
Copy link
Contributor

@frankiebee frankiebee left a comment

Choose a reason for hiding this comment

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

great thank you

@frankiebee frankiebee merged commit d16d6f4 into MetaMask:develop Jun 28, 2019
@akshitkrnagpal akshitkrnagpal deleted the fix-github-#6707 branch June 30, 2019 20:51
This was referenced Jul 1, 2019
Merged
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@frankiebee frankiebee frankiebee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metamask stops working if gas is passed as an object
4 participants
@akshitkrnagpal @whymarrh @frankiebee @Gudahtt