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

Unify to prefix messages for signing #1473

Merged
merged 17 commits into from
Oct 31, 2019

Conversation

nambrot
Copy link
Contributor

@nambrot nambrot commented Oct 24, 2019

Description

Up till this point, we had two different ways of signing messages:

  1. The official Ethereum way by prefixing a message with a special ethereum prefix and hashing that (https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign). We built tooling around that with Signatures.sol and use it to for example authorize signing keys.
  2. The more "low-level" way of signing the message directly, mostly used for Attestations.

This PR moves the latter to the former so that we have a predictable way of signing messages. This will especially come in handy if we decide in the future to sign attestations with a key stored in a node (vs. explicitly specifying the private key right now).

Tested

  • Unit tests

Related issues

Backwards compatibility

  • This is not backwards compatible for Attestations, so should ideally go in with all the other breaking changes

@@ -3,7 +3,18 @@ import * as ganache from '@celo/ganache-cli'
import * as path from 'path'

const MNEMONIC = 'concert load couple harbor equip island argue ramp clarify fence smart topic'

export const ACCOUNT_PRIVATE_KEYS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

what are these for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are the corresponding private keys for the accounts in ganache. They are the same from the protocol tests. Sometimes we need access to the private keys to do some of these actions, like comparing if signatures are equivalent. Though this would also meanz that we can now get ridd of the private keys in the protocol tests since we can just "natively" sign

Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

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

The /utils package changes LGTM. Don't have enough context to review the rest

}

// Uses a native function to sign (as signFn), most commonly `web.eth.sign`
export async function signMessageNatively(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe signMessageNoPrefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The native signing does prefix the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change signMessageForMessageHash to signMessageNoPrefix

}

export function parseSignature(messageHash: string, signature: string, signer: string) {
export function parseSignatureForMessageHash(
Copy link
Contributor

Choose a reason for hiding this comment

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

parseSignatureNoPrefix?

// This only really tests signatureUtils in @celo/utils, but is tested here
// to avoid the web3/ganache setup in @celo/utils
testWithGanache('Signing', (web3) => {
it('signs a message the same way natively and with an explicit private key', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

signs a message the same way via RPC and with an explicit private key

expect(nativeSignature).toEqual(simulatedSignature)
})

it('signs a message that was hashed the same way natively and with an explicit private key', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this is testing that's different from the previous test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a clarifying comment. Basically the difference is that if message is a hex-string, an implementation might not properly calculate the length of the message in the prefix

// This test checks that the prefixing in `signMessage` appropriately considers hex strings
// as bytes the same way the native RPC signing would

@asaj asaj assigned nambrot and unassigned asaj Oct 25, 2019
@codecov
Copy link

codecov bot commented Oct 25, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@7310ab8). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1473   +/-   ##
=========================================
  Coverage          ?   73.72%           
=========================================
  Files             ?      277           
  Lines             ?     7422           
  Branches          ?      955           
=========================================
  Hits              ?     5472           
  Misses            ?     1838           
  Partials          ?      112
Flag Coverage Δ
#mobile 73.72% <ø> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7310ab8...be18321. Read the comment docs.

@nambrot nambrot assigned asaj and unassigned nambrot Oct 25, 2019
Copy link
Contributor

@asaj asaj left a comment

Choose a reason for hiding this comment

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

LGTM pending naming fix

}

export function signMessage(message: string, privateKey: string, address: string) {
return signMessageNoPrefix(hashMessageWithPrefix(message), privateKey, address)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm maybe my previous suggestion was misguided because this line is really confusing:
signMessageNoPrefix(hashMessageWithPrefix

}

export function parseSignature(message: string, signature: string, signer: string) {
return parseSignatureNoPrefix(hashMessageWithPrefix(message), signature, signer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@asaj asaj assigned nambrot and unassigned asaj Oct 30, 2019
@nambrot nambrot added the automerge Have PR merge automatically when checks pass label Oct 30, 2019
@celo-ci-bot-user celo-ci-bot-user merged commit 5bd1768 into master Oct 31, 2019
@celo-ci-bot-user celo-ci-bot-user deleted the nambrot/use-signature-prefix branch October 31, 2019 20:59
aaronmgdr added a commit that referenced this pull request Nov 2, 2019
* master: (62 commits)
  Fix e2e on CI (#1537)
  Allow a specified address to disable/enable the Exchange  (#1467)
  Avoid re-encrypting key files with yarn keys:encrypt command (#1560)
  Support protocol hotfixing (#613)
  Point e2e tests back (#1562)
  Refactor to Accounts.sol (#1392)
  Add selectIssuers Transaction (#1327)
  [Wallet] Get React Native Hot Reloading Working (#1551)
  Unify to prefix messages for signing (#1473)
  [Wallet] Improve error handling around account creation and keystore ops (#1497)
  Add CI test for checking licenses and misc package.json cleanup (#1550)
  [Wallet] Implement SMS invite on iOS (#1541)
  CI: brings back to master (#1554)
  Validators: uses Ethereum address for proof of possession (#1494)
  Validate Attestation Requests (#1468)
  Rename hosted node references to forno (#1511)
  Bump rubyzip from 1.2.3 to 1.3.0 in /packages/mobile (#1508)
  Added txpool family to geth apis. Sorted geth cmd options (#1462)
  [Wallet] Fix yarn dev command for running android (#1534)
  [Wallet] iOS info plist changes and version bump (#1539)
  ...

# Conflicts:
#	yarn.lock
aaronmgdr added a commit that referenced this pull request Dec 5, 2019
* master: (73 commits)
  Fix Ethstats Image reference (#1577)
  EU Cookies Behavior Change (#1447)
  [verifier] Upgrade to RN 61 (#1572)
  [Wallet] Update link styles and Implement VerificationEducationScreen (#1565)
  [wallet] Added native phone picker (#1310)
  [Wallet] Set up new verification screen skeletons (#1563)
  Bump e2e test migrate numbers where needed (#1567)
  [Wallet] Create new carousel component (#1555)
  [Wallet] Protect Backup Key and Safeguards with PIN (#1556)
  Increase ganache gas limit (#1569)
  Re-work locked gold requirements for validators and groups (#1474)
  Fix e2e on CI (#1537)
  Allow a specified address to disable/enable the Exchange  (#1467)
  Avoid re-encrypting key files with yarn keys:encrypt command (#1560)
  Support protocol hotfixing (#613)
  Point e2e tests back (#1562)
  Refactor to Accounts.sol (#1392)
  Add selectIssuers Transaction (#1327)
  [Wallet] Get React Native Hot Reloading Working (#1551)
  Unify to prefix messages for signing (#1473)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users should be able to sign with the same format
5 participants