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

Bump EigenSDK to v0.2.0-beta.1 for compatibility with new Holesky Deploy #45

Merged
merged 12 commits into from
Jan 13, 2025

Conversation

afkbyte
Copy link
Contributor

@afkbyte afkbyte commented Jan 9, 2025

Motivation

AVS were seeing the following error when running the current version of avs sync post-holesky upgrade:

Failed to create ELChainReader: Failed to fetch Slasher address: execution reverted)"

Solution

Bumps EigenSDK to v0.2.0-beta.1

Open questions

We don't include the AllocationManager in the legacy NewEigenlayerContractBindings see: https://github.com/Layr-Labs/eigensdk-go/blob/a0cf13a02ec6777ec40c237e8b2c48ee6b3ce78e/chainio/clients/elcontracts/bindings.go#L133 which is used to create the bindings for the avsRegistry reader and writer. This means that this new branch should work on both holesky and mainnet - but was this intentional?

Edit: Realized it probably is because the AllocationManager can be derived from the DelegationManager

@afkbyte afkbyte requested a review from samlaf January 9, 2025 16:10
@afkbyte afkbyte changed the title [Do not merge] Bump EigenSDK to v0.2.0-beta.1 for compatibility with new Holesky Deploy [Do not merge yet] Bump EigenSDK to v0.2.0-beta.1 for compatibility with new Holesky Deploy Jan 9, 2025
@afkbyte afkbyte changed the title [Do not merge yet] Bump EigenSDK to v0.2.0-beta.1 for compatibility with new Holesky Deploy Bump EigenSDK to v0.2.0-beta.1 for compatibility with new Holesky Deploy Jan 9, 2025
Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but I'm out of the loop on eigenlayer and sdk stuff, so tagging @shrimalmadhur for another review.

Also @ian-shim to make sure this change is fine for eigenda.

integration_test.go Outdated Show resolved Hide resolved
integration_test.go Outdated Show resolved Hide resolved
integration_test.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
Comment on lines 19 to 20
AvsReader avsregistry.ChainReader
AvsWriter avsregistry.ChainWriter
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this change exactly? Haven't touched the sdk in a while so not sure. Is this the change that fixes the "no slasher founder" issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah so changing to the new avs chainreader and chainwriter fixes the issue because they're no longer checking for the slasher which no longer exists on holesky

Can see the diff: https://github.com/Layr-Labs/eigensdk-go/pull/342/files
Screenshot 2025-01-09 at 11 37 14 AM

Choose a reason for hiding this comment

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

Oh this is interface renaming
We had bad naming - avsRegistry.AvsRegistryChainReader. We replaced it with avsRegistry.ChainReader

Copy link

@shrimalmadhur shrimalmadhur Jan 9, 2025

Choose a reason for hiding this comment

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

And what afk said - this version of sdk removes slasher contract binding generation so fixes the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks!

Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

LGTM I'll keep my stamp, but would still prefer we wait for @ian-shim's approval in case this could cause issues for EigenDA.

Copy link

@shrimalmadhur shrimalmadhur left a comment

Choose a reason for hiding this comment

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

lgtm

@afkbyte afkbyte merged commit 44b673f into master Jan 13, 2025
1 check passed
@afkbyte afkbyte deleted the testnet branch January 13, 2025 15:25
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