feat: revised LightAccount v2 1271 sigs #614
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Updates the `signMessage` and `signTypedData` implementations for `LightAccount` v2. The updated version no longer attempts to use nested typed data to display the original message to the user, as doing so creates a security risk in which valid signing requests can be disguished in a way that concels their original contents. Instead, the new `LightAccount` v2 uses the same signing mechanism as v1.1.0, except that it also prepends an enum indicating if the owner is an EOA or smart contract account, as a way to both save gas and to provide more accurate gas estimates.
dphilipson
requested review from
moldy530,
rthomare,
dancoombs,
mokok123,
avasisht23 and
denniswon
as code owners
jaypaik
approved these changes
Apr 23, 2024
| @@ -242,7 +231,7 @@ export async function createLightAccountBase({ | |||
| case "v1.1.0": | |||
| return signWith1271WrapperV1(hashMessage(message)); | |||
There was a problem hiding this comment.
Should this be awaited too?
There was a problem hiding this comment.
return x and return await x do the same thing (unless in a try-block), so the linter would probably complain at me.
moldy530
reviewed
Apr 23, 2024
| @@ -242,7 +231,7 @@ export async function createLightAccountBase({ | |||
| case "v1.1.0": | |||
| return signWith1271WrapperV1(hashMessage(message)); | |||
| case "v2.0.0": | |||
| const signature = await signMessageV2({ message }); | |||
| const signature = await signWith1271WrapperV1(hashMessage(message)); | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
moldy530
approved these changes
Apr 23, 2024
yea we should delete them and ideally open a pr with viem to have them export them |
|
Yeah, no need to include functions we're not using, we can always get them back from version control if we need them again. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates the
signMessageandsignTypedDataimplementations forLightAccountv2. The updated version no longer attempts to use nested typed data to display the original message to the user, as doing so creates a security risk in which valid signing requests can be disguished in a way that concels their original contents.Instead, the new
LightAccountv2 uses the same signing mechanism as v1.1.0, except that it also prepends an enum indicating if the owner is an EOA or smart contract account, as a way to both save gas and to provide more accurate gas estimates.PR-Codex overview
This PR refactors the
createLightAccountBasefunction inbase.tsto remove usage ofgetErc1271SigningFunctionsand directly callsignWith1271WrapperV1.Detailed summary
getErc1271SigningFunctionsimport and direct calls tosignWith1271WrapperV1.hashDomain.tsandhashTypedData.ts.