ceramicnetwork / js-did

A simple interface to interact with DIDs that conform to the DID-provider interface.
Other
95 stars 28 forks source link

fix: eth personal sign hex message #148

Closed zachferland closed 1 year ago

zachferland commented 1 year ago

personal_sign spec expects a hex encoded utf8 message, most providers do seem to support both hex & utf8 strings (ie metamask provider and we havent had any other reported errors from common providers), plus most libraries (ethers, web3) accept both and encode as hex before making provider call

this will provide better general support, esp any other providers that may just follow spec

This will resolve issue - https://github.com/ceramicnetwork/js-did/issues/131

zachferland commented 1 year ago

@oed the existing integration tests use pkh-ethereum, they still pass and most importantly the verifiers have not changed in any way and still pass (will not be breaking, nor have to be coordinated with ceramic nodes in anyway)

can pretty quickly try with metamask as well here to see that utf8 string is presented to user still - https://metamask.github.io/api-playground/api-documentation/#personal_sign, the the rpc request below, message is hex encode siwe string

{
    "jsonrpc": "2.0",
    "method": "personal_sign",
    "params": [ 
"0x6d796170702077616e747320796f7520746f207369676e20696e207769746820796f757220457468657265756d206163636f756e743a0a3078343333363730393963643534346339656463623266333265646333336663383061343737373734610a0a476976652074686973206170706c69636174696f6e2061636365737320746f20736f6d65206f6620796f75722064617461206f6e20436572616d69630a0a5552493a206469643a6b65793a7a364d6b726d636b6445696f6161444a5742767844667a354c38466262646b323957534e3972775863586732455553390a56657273696f6e3a20310a436861696e2049443a20350a4e6f6e63653a206145434878674e6f74790a4973737565642041743a20323032332d30322d32335432303a34373a32372e3538325a0a45787069726174696f6e2054696d653a20323032332d30332d30325432303a34373a32372e3538325a0a5265736f75726365733a0a2d20636572616d69633a2f2f2a",
        "your-eth-address"
    ],
    "id": 0
}
oed commented 1 year ago

@zachferland This seems like the kind of change that would be good to test with a lot of wallets to ensure that it actually work? Only testing metamask seems a bit risky.

Can we create a list with 10 or so of the most popular wallets and test it with them? This is the kind of thing we need to get better at testing anyway.

zachferland commented 1 year ago

@oed dont think its necessary here because 1) it is specified as hex encoded utf8 2) major libraries like web3/ethers already convert every personal_sign message arg into hex before the provider request, ie https://github.com/web3/web3.js/blob/2b3fb3a231534e94c7b3892f37ff77100067b9e9/packages/web3-core-helpers/src/formatters.js#L221

would assume those libraries work fine with most all providers, i would say its less likely to work with wallets in current form

zachferland commented 1 year ago

thanks, but yeah generally pkh libs and integration tests need to be improved here at some point