[Bug] SDK's `signTypedData` calls for Ledger Wallets are broken

[medhakothari]

Expected Behavior

signTypedData calls for Ledger Wallets works in the SDK.

Current Behavior

Discord Link describing this issue.

it looks like contractkit 1.0.1+ has broken signTypedData calls for Ledger Wallets.

The reason is because this call, doesn't actually exist in hw-app-eth version ~5.11, which you had to revert to because of the other bug.

[zviadm]

fyi, I finally found when changes where made to EIP712 signing. this diff just changed both signing and verifying in a completely non-backwards compatible manner:

https://github.com/celo-org/celo-monorepo/commit/b84e1082bcdb293bb512647e17825f59af86cbaf

This is also when it introduced signing EIP712 through Ledger directly which as far as I can tell is just broken now with contractkit 1.x.x, but i am now not even sure if it was ever working properly tbh after this commit.

Has anyone tested the signTypedData calls through Ledger wallet after that change?

[zviadm]

hey, I just verified this. Indeed LedgerWallet signTypedData call was already broken in contractkit 0.4.18. So this has most likely been completely broken since https://github.com/celo-org/celo-monorepo/commit/b84e1082bcdb293bb512647e17825f59af86cbaf @bowd

[zviadm]

The problem is pretty obvious in hindsight, the call is completely broken. Code looks like this:

      const domainSeparator = structHash('EIP712Domain', typedData.domain, typedData.types)
      const hashStructMessage = structHash(
        typedData.primaryType,
        typedData.message,
        typedData.types
      )

      const sig = await this.ledger!.signEIP712HashedMessage(
        await this.getValidatedDerivationPath(),
        domainSeparator,
        hashStructMessage
      )

structHash returns a 'Buffer' type.

https://www.npmjs.com/package/@ledgerhq/hw-app-eth?activeTab=readme#signeip712hashedmessage

signEIP712HashedMessage takes in strings not Buffers as arguments.

(cc: @bowd )

[bowd]

Yup great catch looks like we got the API wrong there. That's what you get when you don't have typescript declaration files for some plugins 😅.

Luckily the fix looks pretty simple easy but we need to do some testing on it to make sure that it actually works this time, and as luck would have it I actually have access to a ledger now 🎉

[medhakothari]

Update on this: Bogdan and I dug into this more this week and the solution doesn't seem to be that simple The main issue is that the necessary EIP712 signing function doesn't exist in the version of the @ledgerhq/hw-app-eth package we're using (which brings up another pro of switching to #6495) If we upgrade to the latest, we run into the issue mentioned here #6496, which is why we downgraded in the first place. Will need to reprioritize this ticket for the upcoming sprint.

[zviadm]

@medhak1 I think it is important to at least have signTypedData function throw a descriptive error for Ledger wallet. (i.e. something like "this function is not implemented!").

Debugging this issue was quite difficult, and I wouldn't want someone else to have to go through the same process if they decide to use this functionality. Since it is completely broken right now, adding a descriptive error should be better than leaving it as is. And adding an error should be pretty simple change that can be pushed with next contractkit version.

[medhakothari]

@zviadm Yah this makes a lot of sense, that's something we can probably push out today or tomorrow. I appreciate all the time you spent debugging this by the way!