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

Fix some MetaMask bugs #951

Merged
merged 13 commits into from
Feb 21, 2020
Merged

Fix some MetaMask bugs #951

merged 13 commits into from
Feb 21, 2020

Conversation

nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Feb 20, 2020

  • Signing messages is different on MetaMask than Truffle, so need to call deriveKeyPairFromSignatureMetamask (from Crypto.js) when commiting and revealing votes, now commit, reveal work well
  • Reading past event logs (via web3.contract.getPastEvents) doesn't work all the time, notably it works sometimes for reading VoteRevealed events but has never worked for PriceResolved. These events are needed to retrieve rewards and show price resolution results, respectively. Therefore, I have added checks to detect if the user is using a metamask provider and to subsequently block certain functionality if so.
  • I have added messaging to communicate to the user that metamask may not work for certain functionality
  • I forked node-metamask and fixed a bug where it does not properly convert scientific notation into numbers, notably when calling web3.eth.getBalance() on an account with a lot of ETH (e.g. 10,000 ETH), it retrieves this result as "1e+22" instead of "10000....000" which causes downstream bignumber errors. I made a PR into the main node-metamask repo about this.

This is part of #901

Resolves #953

@nicholaspai nicholaspai added the bug Something isn't working label Feb 20, 2020
@nicholaspai nicholaspai added this to the Voting CLI milestone Feb 20, 2020
@mrice32
Copy link
Member

mrice32 commented Feb 21, 2020

@chrismaree could you take a first pass on this?

Copy link
Member

@chrismaree chrismaree left a comment

Choose a reason for hiding this comment

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

I went through this and ran it locally. All works as expected except for an issue which is unrelated to this PR. I created an issue #953 . Also a number of persistent event issues remain but this is documented in #901 specifically when going through a commit-reveal process

common/VotingUtils.js Outdated Show resolved Hide resolved
core/scripts/cli/voting/displayStatus.js Outdated Show resolved Hide resolved
Signed-off-by: Nick Pai <[email protected]>
common/VotingUtils.js Outdated Show resolved Hide resolved
common/VotingUtils.js Outdated Show resolved Hide resolved
Signed-off-by: Nick Pai <[email protected]>
@nicholaspai nicholaspai merged commit 8e3a298 into master Feb 21, 2020
@nicholaspai nicholaspai deleted the cli-fix-metamask branch February 21, 2020 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI utility shows wrong etherscan links when generating tx
3 participants