feat: Add EIP-712 management in prepareMessageToSign #892
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 909fef0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
common
Screenshots: ✅
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
libs/ledger-live-common/src/families/ethereum/hw-signMessage.test.ts
Outdated
Show resolved
Hide resolved
|
There seem to be an issue with the update in |
Codecov Report
@@ Coverage Diff @@
## develop #892 +/- ##
===========================================
+ Coverage 47.82% 47.86% +0.03%
===========================================
Files 650 650
Lines 29114 29146 +32
Branches 7529 7541 +12
===========================================
+ Hits 13925 13951 +26
- Misses 15131 15137 +6
Partials 58 58
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Fixed in 75cb580 |
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
…o avoid confusion Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
sprohaszka-ledger
force-pushed
the
feat/LIVE-3180-EIP-712
branch
from
0a77f64 to
77aa652
Compare
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
| import hwSignMessage from "./hw-signMessage"; | ||
| import { EIP712Message } from "@ledgerhq/hw-app-eth/lib/modules/EIP712"; | ||
| import testEIP712Message from "@ledgerhq/hw-app-eth/tests/sample-messages/0.json"; | ||
| import ethSignMessage, { prepareMessageToSign } from "./hw-signMessage"; |
There was a problem hiding this comment.
I guess here we can just
import { signMessage, prepareMessageToSign } from "./hw-signMessage";And replace all hwSignMessage by signMessage ?
| let parsedMessage = message; | ||
| if (typeof message === "string") { | ||
| parsedMessage = tryConvertToJSON(message); | ||
| } |
There was a problem hiding this comment.
Is this condition necessary ? JSON.parse({}) should throw and be catched and return the original message, so we might be able to update the typings of tryConvertToJSON to allow for more "all-in-one" behaviour ?
There was a problem hiding this comment.
message can be of the type string or EIP712Message. In the latter case, we can't and don't want to try to convert it into a JSON.
I added typing info in the method to be clearer.
There was a problem hiding this comment.
What I meant was to potentially change tryConvertToJSON into something like this:
function tryConvertToEIP712(message: string | Record<string, any>): string | EIP712Message {
const maybeParsedMessage = (() => {
try {
return JSON.parse(message as string) as Record<string, any>;
} catch {
return message;
}
})();
return isEIP712Message(maybeParsedMessage)
? maybeParsedMessage
: message.toString();
}So:
- if message is a random
stringit would throw on parse and return message as astring - if message is a stringified random
objectit would fail onisEIP712Messageand return message as astring - if message is an
EIP712stringified object it would returnmaybeParsedMessageas anEIP712Message - if message is an
EIP712Messageit would throw on parse, succeed onisEIP712Messageand returnmaybeParsedMessageas anEIP712Message
Only other adjustement to make would to add a type guard toisEIP712Messagethat themessageparam is an object first.
But again, I'm nit picking here, and it's just another abstraction, nothing major, feel free to ignore. 👍
Signed-off-by: Stéphane Prohaszka <[email protected]>
|
Works well with wallet-connect and same for EIP 191 messages |
desirendr
added
translations
Signed-off-by: Stéphane Prohaszka [email protected]
📝 Description
Add EIP-712 support on WebPlatformPlayer.
Rework some unused properties in
TypesMessageDataand update impacted WalletConnect current implementation.❓ Context
commonledgerjs-eth✅ Checklist
📸 Demo
🚀 Expectations to reach
The goal, beside Platform capacity to manage EIP-712 call, is to remove unused code and keep eth logic inside dedicated modules (i.e.
ledgerjs/package/hw-app/ethandcommon/families/etherereum).Also, leverage as much as possible type to use typescript validation build system to check for unwanted behavior.
It can be tested by running LLD with
EXPERIMENTAL_EIP712=1and using the ethereum app available on provider 3