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

fix: signature utils #1015

Open
wants to merge 2 commits into
base: slashing-magnitudes-fixes
Choose a base branch
from

Conversation

0xClandestine
Copy link
Member

@0xClandestine 0xClandestine commented Jan 11, 2025

Changes:

  • Dynamic Domain Separator: SignatureUtils.domainSeparator() is now recomputed for each signature verification. This eliminates the need for storing initial values in storage or as immutables, which is important for beacon proxy support.

  • Version Bump Command: Introduced make bump-version VERSION=2, which automatically updates the version function's return values.

  • Version Fn + Constructor Param: Adds an immutable oz ShortString that's set in the constructor.

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

Can we sanity check that the domain separator of the DM/RC/AVSD is based on the proxy and not the implementation

@0xClandestine
Copy link
Member Author

Can we sanity check that the domain separator of the DM/RC/AVSD is based on the proxy and not the implementation

Oops, forgot that. Will do.

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

Comments regarding version number

bin/bump-version.sh Outdated Show resolved Hide resolved
src/contracts/core/AVSDirectory.sol Outdated Show resolved Hide resolved
src/contracts/core/DelegationManager.sol Outdated Show resolved Hide resolved
src/test/unit/RewardsCoordinatorUnit.t.sol Outdated Show resolved Hide resolved
@0xClandestine
Copy link
Member Author

0xClandestine commented Jan 13, 2025

Check storage layout ci failing due to SignatureUtils -> SignatureUtilsMixin.

@wadealexc wadealexc force-pushed the slashing-magnitudes-fixes branch from 1427a85 to 403e0a1 Compare January 13, 2025 19:58
@0xClandestine 0xClandestine force-pushed the fix/signature-utils branch 2 times, most recently from 4189591 to 6d5d06b Compare January 16, 2025 19:23
Makefile Outdated Show resolved Hide resolved
Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

So far looks good, we should also add SemverMixin to the EigenPodManager and StrategyFactory.

We may want to also add to all Strategies and EigenPods too (can talk in standup about this one).

script/configs/mainnet/mainnet-addresses.config.json Outdated Show resolved Hide resolved
@@ -53,6 +53,10 @@ library Env {
* env
*/

function version() internal view returns (string memory) {
return _envString("version");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you coordinate with @jbrower95 to make sure this is a parameter in the Zeus config that the upgrade scripts can access

Copy link
Member Author

Choose a reason for hiding this comment

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

in progress

Copy link
Collaborator

Choose a reason for hiding this comment

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

To add to this, but maybe overkill, we may want to have zeus config to have semver for each contract. Some of our upgrades will touch just a few contracts so the versions will be different at a point in time

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh that's a good point, so rather than a single config param, there's one per contract?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ya that would make sense to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Just noting we decided against this...

Ahh that's a good point, so rather than a single config param, there's one per contract?

src/contracts/core/DelegationManager.sol Outdated Show resolved Hide resolved
src/contracts/mixins/SemVerMixin.sol Outdated Show resolved Hide resolved
src/contracts/interfaces/ISignatureUtilsMixin.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah not entirely sure if we want to change EigenStrategy here. I figure the Eigen/EigenStrategy changes will take place in its own separate upgrade.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We upgraded the EigenStrategy in the initial upgrade, so we should be good. Eigen can be separate:

deployImpl({
name: type(EigenStrategy).name,
deployedTo: address(new EigenStrategy({
_strategyManager: Env.proxy.strategyManager(),
_pauserRegistry: Env.impl.pauserRegistry()
}))
});

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

LGTM, approving. Just need to coordinate with @jbrower95 on the Zeus changes

@ypatil12
Copy link
Collaborator

Oh, can we also update the storage diff for sanity?

@ypatil12
Copy link
Collaborator

LGTM, approving. Just need to coordinate with @jbrower95 on the Zeus changes

Spoke with @jbrower95, we're just going to add a Zeus helper solidity script to tick the version. Will be done in a separate PR

Copy link
Collaborator

@ypatil12 ypatil12 left a comment

Choose a reason for hiding this comment

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

Blocking for now - let's wait to merge until we have all audit fixes in

refactor: review changes

refactor: review changes

feat: remaining semver

feat: remaining semver

refactor: review changes

refactor: review changes

fix: ci

refactor: remove make cmd

fix: ci

fix: ci

feat: add `SemVerMixin` (#1034)

* feat: add `SemVerMixin`

* refactor: add `_majorVersion` helper for `SignatureUtils`

* test: passing

* test: passing

* refactor: rename EIP712_VERSION

* refactor: review changes

chore: forge fmt

refactor: natspec improvements

chore: forge fmt

fix: eip712 typehash

refactor: remove version overrides

refactor: remove make bump-version

refactor: immutable version

refactor: remove unused import

chore: `make bump-version VERSION=1`

refactor: `SignatureUtils` -> `SignatureUtilsMixin`

fix: bump-version.sh

chore: `make bump-version VERSION=1.0.3`

feat: add `make bump-version`

feat: add version fn

refactor: cleanup

fix: signature utils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants