loom-js icon indicating copy to clipboard operation
loom-js copied to clipboard

always use eth_personal_sign

Open lukezhangstudio opened this issue 6 years ago • 3 comments

Because of the way Ethers.js implements a workaround for metamask, we have been effectively using personal.sign. Let's just switch to using personal_sign explicitly in case someone imports addressmapper without using dposuser.

Our own workaround: https://github.com/loomnetwork/loom-js/blob/a128250652ba044e6dfdf7ffabc8a1f956a44b93/src/solidity-helpers.ts#L24

lukezhangstudio avatar Jun 19 '19 06:06 lukezhangstudio

Example of where we use .sign instead of personal.sign https://github.com/loomnetwork/loom-js/blob/02ef5d542009a94a591bd39b4704748b0a793e1b/src/contracts/address-mapper.ts#L49

lukezhangstudio avatar Jun 19 '19 06:06 lukezhangstudio

@lukezhangstudio that function is passing an interface for IEthereumSigner, so in order to use the personal.sign the user should pass the EthersSigner to that function

async addIdentityMappingAsync(
  from: Address,
  to: Address,
  ethersSigner: IEthereumSigner
): Promise<Uint8Array | void> {
  const mappingIdentityRequest = new AddressMapperAddIdentityMappingRequest()
  mappingIdentityRequest.setFrom(from.MarshalPB())
  mappingIdentityRequest.setTo(to.MarshalPB())

  const hash = soliditySha3(
    {
      type: 'address',
      value: from.local.toString().slice(2)
    },
    {
      type: 'address',
      value: to.local.toString().slice(2)
    }
  )

  const sign = await ethersSigner.signAsync(hash)
  mappingIdentityRequest.setSignature(sign)

  return this.callAsync<void>('AddIdentityMapping', mappingIdentityRequest)
}

https://github.com/loomnetwork/loom-js/blob/877edfc6c5a50eb5ce432b5c798026d5cbd60256/src/solidity-helpers.ts#L47

Instead of Web3Signer which not implements personal_sign

https://github.com/loomnetwork/loom-js/blob/877edfc6c5a50eb5ce432b5c798026d5cbd60256/src/solidity-helpers.ts#L79

eduardonunesp avatar Sep 10 '19 23:09 eduardonunesp

@lukezhangstudio could you please confirm if my comment above makes sense?

https://github.com/loomnetwork/loom-js/blob/877edfc6c5a50eb5ce432b5c798026d5cbd60256/src/tests/e2e/address-mapper-tests.ts#L47

eduardonunesp avatar Sep 11 '19 20:09 eduardonunesp