Skip to content
This repository has been archived by the owner on Feb 20, 2019. It is now read-only.

Use eth_signTypedData for authenticating #2

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

sprice
Copy link
Contributor

@sprice sprice commented May 15, 2018

馃毃Do Not Merge 馃毃

We are waiting until eth_signTypedData is widely supported

Corresponding web app PR https://github.com/codex-protocol/web.codex-title-viewer/pull/9

@Anaphase
Copy link
Member

Reading through the comments (and the disclaimer up top) on the Medium article, it seems like this might not be the best idea to implement right away as the API might change?

Copy link
Contributor

@johnhforrest johnhforrest left a comment

Choose a reason for hiding this comment

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

Will approve after the package-lock stuff is sorted out

import RestifyErrors from 'restify-errors'

import models from '../../models'
import config from '../../config'
import logger from '../../services/logger'

const hashedPersonalMessage = ethUtil.hashPersonalMessage(ethUtil.toBuffer(config.personalMessageToSign))
const { typedDataToSign } = config
Copy link
Member

Choose a reason for hiding this comment

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

Remove this line and just use config.typedDataToSign below, i.e.:

const recoveredAddress = ethSigUtil.recoverTypedSignature({
  sig: request.parameters.signedData,
  data: config.typedDataToSign,
})

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'm doing something similar on the front end and it complained that I wasn't destructuring. Happy to make this change though.

// NOTE: signedData should not have the 0x prefix as Joi will validate it as
// a raw Buffer
signedData: Joi.binary().encoding('hex').length(65).required(),
signedData: Joi.string().length(132).required(),
Copy link
Member

Choose a reason for hiding this comment

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

Will length always be 132? Is there any documentation/source code you can point me to that verifies this will always be 132? Could always just remove the length too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will let recoverTypedSignature deal with length if it needs to.

src/config.js Outdated
{
type: 'string',
name: 'Sign In',
value: 'Sign in to Codex Title Viewer',
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this message similar to what it was before?

Please sign this message to authenticate with the Codex Title Viewer.

Or is that too verbose 馃

const userAddressBuffer = ethUtil.toBuffer(request.parameters.userAddress)
const senderAddressBuffer = ethUtil.publicToAddress(publicKey)
const recoveredAddress = ethSigUtil.recoverTypedSignature({
data: typedDataToSign,
Copy link
Member

Choose a reason for hiding this comment

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

Can you flip these two lines so they're in descending order by length, i.e.:

const recoveredAddress = ethSigUtil.recoverTypedSignature({
  sig: request.parameters.signedData,
  data: config.typedDataToSign,
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to. Is longest parameter name first part of our style guide? Haven't come across that before :)

Copy link
Member

Choose a reason for hiding this comment

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

Not really I guess, it's just my personal preference. It's also kind of arbitrary how I decide that, it's just whatever "looks" best. I typically try to cascade things like this though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants