feat(EIP712): parsing of TypedData payload; encoding + hashing; #833

JeneaVranceanu · 2023-10-18T07:52:29Z

Summary of Changes

Implementation of EIP712 TypedData payload parsing;
Payload encoding & hashing.

Upd: this PR is focused on giving the ability to receive a JSON payload via eth_signTypedData RPC call, parse this payload, do some validations (though, validations can be improved for sure), encode message (a message is one of the fields of received JSON), hash message and according to given types (types also part of the received JSON and can be arbitrary), and prepare a special EIP712 hash that's used for signing of the received payload.

In order to parse and sign this payload you as a user must do the following:

let eip712TypedData = try EIP712Parser.parse(receivedPayloadFromRPC)
let signature = try Web3Signer.signEIP712(
    eip712TypedData,
    keystore: keystore,
    account: account,
    password: password)

Test Data or Screenshots

By submitting this pull request, you are confirming the following:

I have reviewed the Contribution Guidelines.
I have performed a self-review of my own code.
I have updated my repository to match the develop branch.
I have included test data or screenshots that prove my fix is effective or that my feature works.
I have checked that all tests work and swiftlint is not throwing any errors/warnings.

…, tests;

…or EIP712TypedData

yaroslavyaroslav · 2023-10-19T08:52:46Z

@JeneaVranceanu I wonder is it ready be reviewed now, or there would be some fixes added?

…not used for EIP712

JeneaVranceanu · 2023-10-19T11:19:42Z

@JeneaVranceanu I wonder is it ready be reviewed now, or there would be some fixes added?

I think it makes sense to look into the old EIP712 implementation that we already had (and maybe even remove it 👀).

JeneaVranceanu · 2023-10-19T11:20:13Z

I'll ping you by the end of this day with final thoughts/changes.

JeneaVranceanu · 2023-10-19T21:46:50Z

@yaroslavyaroslav No more changes for this PR.
I've updated the documentation for EIP712Parser and the next steps will be:

Check that public class EIP712 covers all types possible;
Update EIP712Domain with all 5 properties according to the standard and add the ability to add more properties through a dictionary (maybe dictionary, not yet decided);
Implement a parsing of custom statically typed objects into EIP712TypedData. As EIP712TypedData has all the encoding and hashing in place it will make sense to transform data from custom objects into EIP712TypedData and then use encode, hash or whatever is needed.

But all of that is for a different PR. This PR will have to wait for a while to get all these updates 🙂
I suggest we review and merge this PR.

awaiskhan1304 · 2023-10-30T08:34:53Z

Tested it with https://react-app.walletconnect.com/ and it is working just perfect.
Thanks @JeneaVranceanu.

yaroslavyaroslav · 2023-10-30T10:57:26Z

No more changes for this PR.

Started to reviewing it this Friday, considering to continue doing this today's evening. Quite tough workload I'm having now, sorry for a delay.

…alizedError type; - updated ParsingError and AbstractKeystoreError with description; - minor refactoring; - added 2 more EIP712 tests;

yaroslavyaroslav · 2023-10-31T10:14:36Z

@JeneaVranceanu we have problems)

JeneaVranceanu · 2023-10-31T12:52:10Z

@JeneaVranceanu we have problems)

Yes, a failing test!
Added this morning. Have to implement support for custom type arrays.
And recursion support is missing.

…n encoding

JeneaVranceanu · 2023-11-03T21:51:01Z

Sources/Web3Core/EthereumABI/ABIParsing.swift

        case invalidJsonFile
-        case elementTypeInvalid
+        case elementTypeInvalid(_ desc: String? = nil)
        case elementNameInvalid
        case functionInputInvalid
        case functionOutputInvalid
        case eventInputInvalid
        case parameterTypeInvalid
        case parameterTypeNotFound
        case abiInvalid


I've decided not to add (_ desc: String? = nil) to all error types as it would trigger more changes across the library.
It's better to do that updated in a separate PR.

JeneaVranceanu · 2023-11-03T21:57:17Z

Sources/web3swift/Utils/EIP/EIP712/EIP712.swift

+// FIXME: this type is wrong - The minimum number of optional fields is 5, and those are
+// string name the user readable name of signing domain, i.e. the name of the DApp or the protocol.
+// string version the current major version of the signing domain. Signatures from different versions are not compatible.
+// uint256 chainId the EIP-155 chain id. The user-agent should refuse signing if it does not match the currently active chain.
+// address verifyingContract the address of the contract that will verify the signature. The user-agent may do contract specific phishing prevention.
+// bytes32 salt an disambiguating salt for the protocol. This can be used as a domain separator of last resort.
 public struct EIP712Domain: EIP712Hashable {


This is something to deal with later.

JeneaVranceanu · 2023-11-04T23:21:20Z

Sources/Web3Core/KeystoreManager/BIP32Keystore.swift

        }
        guard let newNode = rootNode.derive(path: pathAppendix, derivePrivateKey: true) else {
-            throw AbstractKeystoreError.keyDerivationError
+            throw AbstractKeystoreError.keyDerivationError("BIP32Keystore. Failed to derive a new node. Check given parent node and path.")


We really need to revisit all our errors and make sure:

We have description;

The description makes sense to the developers who could get this error.

…nu/web3swift into feat/eip712-dynamic-parsing * 'feat/eip712-dynamic-parsing' of github.com:JeneaVranceanu/web3swift: chore: docs update chore: updated error message

yaroslavyaroslav · 2023-11-17T18:02:57Z

Sources/Web3Core/KeystoreManager/BIP39.swift

        guard
-            size >= 128 && size <= 256 && size.isMultiple(of: 32),
-            let entropy = Data.randomBytes(length: size/8)
+            isCorrectSize,


Although it got better, it still doesn't fit with our convention of guard else formatting.

yaroslavyaroslav · 2023-11-17T18:06:03Z

Sources/Web3Core/KeystoreManager/EthereumKeystoreV3.swift

-        if self.addresses?.count == 1 && account == self.addresses?.last {
-            guard let privateKey = try? self.getKeyData(password) else {
+        if account == addresses?.last {
+            guard let privateKey = try? getKeyData(password) else {


Maybe we can make this bit a bit more straightforward? Like changing the if let above to an another guard statement.

Upd: oh, I see that the execution flow continues after that line:32 throw statement. Maybe we should add at least TODO: refactor me at the very beginning of the whole file, as I believe this function should be split in to, but not this time?

yaroslavyaroslav · 2023-11-17T18:13:36Z

Sources/web3swift/Utils/EIP/EIP712/EIP712Parser.swift

+}
+
+internal struct EIP712TypeArray: Codable {
+    public let types: [String : [EIP712TypeProperty]]


I wonder if this could be a concrete type rather the dictionary?

yaroslavyaroslav · 2023-11-17T18:14:10Z

Sources/web3swift/Utils/EIP/EIP712/EIP712Parser.swift

+}
+
+public struct EIP712TypedData {
+    public let types: [String: [EIP712TypeProperty]]


Same question about the concrete type here.

yaroslavyaroslav · 2023-11-17T18:16:19Z

Sources/web3swift/Utils/EIP/EIP712/EIP712Parser.swift

+        guard let primaryType = json["primaryType"] as? String else {
+            throw Web3Error.inputError(desc: "EIP712Parser. Top-level string field 'primaryType' missing.")
+        }
+        guard let domain = json["domain"] as? [String : AnyObject] else {


Maybe we should add here some at least short reasoning about why it's left here as a dictionary, rather being casted in a concrete type?

yaroslavyaroslav · 2023-11-17T18:30:35Z

Sources/web3swift/Utils/Extensions/String+Extension.swift

+        guard let data = data(using: .utf8) else { return nil }
+        return try data.asJsonDictionary()
+    }
+}


We're still haven't added SwiftLint ruleset here, and I wonder would we ever do this, but I believe that such a set that we're having in a stash contains the rule about keeping one empty line at the very end of each file.

yaroslavyaroslav · 2023-11-17T18:31:14Z

Sources/web3swift/Web3/Web3+Signing.swift

+        guard let signature = try Web3Signer.signPersonalMessage(hash,
+                                                                 keystore: keystore,
+                                                                 account: account,
+                                                                 password: password ?? "",


Is this default password value necessary?

yaroslavyaroslav · 2023-11-17T18:31:30Z

Sources/web3swift/Web3/Web3+Signing.swift

        guard let signature = try Web3Signer.signPersonalMessage(hash,
                                                                 keystore: keystore,
                                                                 account: account,
-                                                                 password: password ?? "")
+                                                                 password: password ?? "",


Is this default password value necessary?

yaroslavyaroslav · 2023-11-17T18:33:05Z

Sources/Web3Core/EthereumABI/ABIParsing.swift

+            case .invalidJsonFile:
+                errorMessage = ["invalidJsonFile"]
+            case .elementTypeInvalid(let desc):
+                errorMessage = ["elementTypeInvalid", desc]


I wonder if this line is a cause that we're messing here with an array of Strings rather a plain String?

yaroslavyaroslav · 2023-11-17T18:36:18Z

Sources/Web3Core/KeystoreManager/AbstractKeystore.swift

+        var errorMessage: [String?]
+        switch self {
+        case .noEntropyError(let additionalDescription):
+            errorMessage = ["Entropy error (e.g. failed to generate a random array of bytes).", additionalDescription]


I wonder if this line is a cause that we're messing here with an array of Strings rather a plain String? Here it looks quite more necessary than in the previous alike code snippet, yet I wonder if it's a best way in terms of code intention clarity to implement such behaviour?

JeneaVranceanu added 6 commits October 18, 2023 10:51

feat(EIP712): parsing of TypedData payload; encoding + hashing;

354589f

featEIP712): parser test

0342f91

chore(EIP712): renamed test file

a9b23fb

feat(EIP712): type encoding, type hashing, circular dependency checks…

ce459b6

…, tests;

chore(EIP712): added link to the tests + implementation source

89595ad

feat(EIP712): impl of encodeData, structHash and signHash functions f…

27d5b61

…or EIP712TypedData

JeneaVranceanu marked this pull request as ready for review October 18, 2023 21:09

fix: typo

07965a1

JeneaVranceanu requested review from janndriessen and yaroslavyaroslav October 18, 2023 21:10

chore(EIP712): new test + moved EIP712 reused data into one file;

5493dd0

fix(EIP712): fixed values in tests since personal-message-hashing is …

4669bfe

…not used for EIP712

chore(EIP712): example for EIP712Parser;

b8e55b2

fix: ParsingError and AbstractKeystoreError are now extensions of Loc…

c01f6a1

…alizedError type; - updated ParsingError and AbstractKeystoreError with description; - minor refactoring; - added 2 more EIP712 tests;

JeneaVranceanu added 5 commits November 3, 2023 10:25

feat: support for eth_signTypedDataV4 payload parsing

41828fb

chore: make parsing errors more granular

c6da90f

fix: order custom subtypes (no primitives!) in alphabetical order whe…

e40c3f8

…n encoding

chore: added new test for Invalid Order Signature error

4990084

fix: updated test case to match the expected result

d69d594

JeneaVranceanu commented Nov 3, 2023

View reviewed changes

chore: updated error message for entropyOf

40a9a3d

JeneaVranceanu commented Nov 3, 2023

View reviewed changes

JeneaVranceanu and others added 4 commits November 4, 2023 00:01

chore: updated error messages for EIP712Parser

cfd8ee9

chore: error message fix

0b0b3fa

chore: updated error message

68d8457

chore: docs update

111d33e

JeneaVranceanu commented Nov 4, 2023

View reviewed changes

JeneaVranceanu added 2 commits November 7, 2023 10:48

fix: EIP712 - change type to AbstractKeystore from BIP32Keystore

e77af0a

Merge branch 'feat/eip712-dynamic-parsing' of github.com:JeneaVrancea…

bd1544d

…nu/web3swift into feat/eip712-dynamic-parsing * 'feat/eip712-dynamic-parsing' of github.com:JeneaVranceanu/web3swift: chore: docs update chore: updated error message

yaroslavyaroslav requested changes Nov 17, 2023

View reviewed changes

fix: parsing and encoding of "bytes"

6cc06db

yaroslavyaroslav approved these changes Feb 7, 2024

View reviewed changes

yaroslavyaroslav merged commit 8ae45e1 into web3swift-team:develop Feb 7, 2024
2 checks passed

JeneaVranceanu deleted the feat/eip712-dynamic-parsing branch February 7, 2024 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(EIP712): parsing of TypedData payload; encoding + hashing; #833

feat(EIP712): parsing of TypedData payload; encoding + hashing; #833

JeneaVranceanu commented Oct 18, 2023 •

edited

Loading

yaroslavyaroslav commented Oct 19, 2023

JeneaVranceanu commented Oct 19, 2023

JeneaVranceanu commented Oct 19, 2023

JeneaVranceanu commented Oct 19, 2023 •

edited

Loading

awaiskhan1304 commented Oct 30, 2023

yaroslavyaroslav commented Oct 30, 2023

yaroslavyaroslav commented Oct 31, 2023

JeneaVranceanu commented Oct 31, 2023

JeneaVranceanu Nov 3, 2023 •

edited

Loading

JeneaVranceanu Nov 3, 2023

JeneaVranceanu Nov 4, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

yaroslavyaroslav Nov 17, 2023

feat(EIP712): parsing of TypedData payload; encoding + hashing; #833

feat(EIP712): parsing of TypedData payload; encoding + hashing; #833

Conversation

JeneaVranceanu commented Oct 18, 2023 • edited Loading

Summary of Changes

Test Data or Screenshots

By submitting this pull request, you are confirming the following:

yaroslavyaroslav commented Oct 19, 2023

JeneaVranceanu commented Oct 19, 2023

JeneaVranceanu commented Oct 19, 2023

JeneaVranceanu commented Oct 19, 2023 • edited Loading

awaiskhan1304 commented Oct 30, 2023

yaroslavyaroslav commented Oct 30, 2023

yaroslavyaroslav commented Oct 31, 2023

JeneaVranceanu commented Oct 31, 2023

JeneaVranceanu Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeneaVranceanu commented Oct 18, 2023 •

edited

Loading

JeneaVranceanu commented Oct 19, 2023 •

edited

Loading

JeneaVranceanu Nov 3, 2023 •

edited

Loading