Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make signing eip 712 compliant #916

Merged
merged 29 commits into from
Sep 10, 2024

Conversation

NicolasRampoldi
Copy link
Contributor

@NicolasRampoldi NicolasRampoldi commented Sep 3, 2024

Important

Added chain param to submit and submit_multiple in the SDK. This is needed to get the BatcherPaymentService contract address.

Important

The version of the Aligned contracts was changed from =0.8.12 to ^0.8.12 to enable compatibility with open-zeppelin 5.0.0 (which is needed to use EIP712). We still use open-zeppelin from eigenlayer-middleware for all the imports but just for the EIP712 we use open-zeppelin 5.0.0.

Note

The GAP variable was lowered by 1 because of the added NONCED_VERIFICATION_DATA_TYPEHASH bytes32 constant.

Note

As stated in the code's comment, VerificationData was used as a bytes32 instead of a VerificationData struct because we don't have the necessary fields in the contract (BatcherPaymentService) to use the VerificationData struct. Also, chain_id is not part of the type hash because it is now part of the domain.

If you want to read more about the EIP-712.

Description

  • This PR makes the NoncedVerificationData signing EIP712 compliant. The EIP712Domain has:

    name: Aligned
    version: 1
    chain_id: <current_chain_id>
    verifying_contract: <current_payment_service contract_addr>
    

Deploying

Devnet:

make anvil_add_type_hash_to_batcher_payment_service

Testnet:

  • Stage or prod variables should be set accordingly on the contracts/scripts/.env file

make upgrade_add_type_hash

Working with Metamask

Screen.Recording.2024-09-09.at.15.22.42.mov

To Test

  • Run make deps.
  • Run everything as usual and make sure it is working properly.

@NicolasRampoldi NicolasRampoldi linked an issue Sep 3, 2024 that may be closed by this pull request
@NicolasRampoldi NicolasRampoldi self-assigned this Sep 3, 2024
Copy link

github-actions bot commented Sep 3, 2024

Changes to gas cost

Generated at commit: af42f188dc0ab725b46a4ee2bf8890f89e239981, compared to commit: 42cc5f549f209d15db31771119b017b39ba5d05d

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
TransparentUpgradeableProxy blsApkRegistry
delegation
stakeRegistry
-21 ✅
-21 ✅
-21 ✅
-1.91%
-0.27%
-0.27%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
TransparentUpgradeableProxy 573,006 (-19,642) blsApkRegistry
delegation
initialize
setBLSPublicKey
stakeRegistry
1,080 (-21)
7,622 (-21)
101,061 (-21)
119,132 (-14)
7,646 (-21)
-1.91%
-0.27%
-0.02%
-0.01%
-0.27%
1,080 (-21)
7,622 (-21)
101,061 (-21)
119,132 (-14)
7,646 (-21)
-1.91%
-0.27%
-0.02%
-0.01%
-0.27%
1,080 (-21)
7,622 (-21)
101,061 (-21)
119,132 (-14)
7,646 (-21)
-1.91%
-0.27%
-0.02%
-0.01%
-0.27%
1,080 (-21)
7,622 (-21)
101,061 (-21)
119,132 (-14)
7,646 (-21)
-1.91%
-0.27%
-0.02%
-0.01%
-0.27%
1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
AlignedLayerServiceManager 4,648,017 (-19,192) createNewTask
receive
56,967 (+12)
21,169 (+6)
+0.02%
+0.03%
76,963 (+57)
44,783 (+10)
+0.07%
+0.02%
77,035 (0)
45,064 (+11)
0.00%
+0.02%
78,140 (-72)
45,064 (+11)
-0.09%
+0.02%
256 (0)
256 (0)
RegistryCoordinatorHarness 5,828,753 (-1,796) initialize 54,717,816 (-33,604) -0.06% 54,717,816 (-33,604) -0.06% 54,717,816 (-33,604) -0.06% 54,717,816 (-33,604) -0.06% 1 (0)
ProxyAdmin 443,159 (-16) upgrade
upgradeAndCall
38,809 (-21)
55,283,451 (-33,712)
-0.05%
-0.06%
38,818 (-21)
55,283,451 (-33,712)
-0.05%
-0.06%
38,821 (-21)
55,283,451 (-33,712)
-0.05%
-0.06%
38,821 (-21)
55,283,451 (-33,712)
-0.05%
-0.06%
4 (0)
1 (0)
StakeRegistryHarness 3,187,927 (-37,767) initializeQuorum 143,101 (-53) -0.04% 162,897 (-53) -0.03% 163,001 (-53) -0.03% 163,001 (-53) -0.03% 192 (0)
BLSApkRegistryHarness 1,837,601 (-29,960) setBLSPublicKey 89,372 (+7) +0.01% 89,372 (+7) +0.01% 89,372 (+7) +0.01% 89,372 (+7) +0.01% 1 (0)
AVSDirectory 1,739,047 (-1,343)
Slasher 849,198 (-3,071)
StrategyManagerMock 1,270,846 (+4,755)
IndexRegistry 1,086,937 (-16,825)
ServiceManagerMock 1,611,464 (-16,658)

@NicolasRampoldi NicolasRampoldi marked this pull request as ready for review September 3, 2024 19:44
# Conflicts:
#	batcher/aligned-sdk/src/core/types.rs
#	contracts/scripts/anvil/state/alignedlayer-deployed-anvil-state.json
#	contracts/src/core/BatcherPaymentService.sol
Copy link
Contributor

@uri-99 uri-99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't finish the review but I leave you this request changes on the mean time

# Conflicts:
#	contracts/scripts/anvil/state/alignedlayer-deployed-anvil-state.json
entropidelic
entropidelic previously approved these changes Sep 6, 2024
# Conflicts:
#	contracts/scripts/anvil/state/alignedlayer-deployed-anvil-state.json
#	docs/3_guides/4_generating_proofs.md
Copy link
Contributor

@MauroToscano MauroToscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed Fee should appear. Nonce would be better if it's not hashed, but I think this in inherited from some weird decision on the code

@MauroToscano MauroToscano merged commit dd19e5f into staging Sep 10, 2024
4 checks passed
@MauroToscano MauroToscano deleted the feat-make-signing-eip-712-compliant branch September 10, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make signing EIP-712 compliant
5 participants