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

Upgrade eigensdk-go and go-ethereum #323

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

ian-shim
Copy link
Contributor

@ian-shim ian-shim commented Mar 7, 2024

Why are these changes needed?

Upgrading eigensdk-go requires upgrading go-ethereum from v1.13.4 to v1.13.12.
This requires several changes

  1. Replace existing go-ethereum logger to slog in eigensdk-go
  2. Replace deprecated SimulatedBackend to simulated.Backend

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@ian-shim ian-shim force-pushed the replace-logger branch 2 times, most recently from 24bbfc0 to fc228e0 Compare March 7, 2024 05:19
@@ -52,7 +50,8 @@ type (
}

simulatedBackend struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

replacing *backends.SimulatedBackend with new package simulated

@@ -9,6 +9,7 @@ import (
)

func TestContractSimulator(t *testing.T) {
t.Skip("Skipping this test after the simulated backend upgrade broke this test. Enable it after fixing the 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.

Deprecated backends.SimulatedBackend is supposed to be backward-compatible, but that doesn't seem to be the case.
This test and indexer/test/indexer_test.go fail when go-ethereum is upgraded to v1.13.12.

Given that we don't rely on built-in indexer for any of the core functionality, I suggest we skip these tests for now.


const (
PathFlagName = "log.path"
LevelFlagName = "log.level"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using one common level for both std and file logs

@ian-shim ian-shim marked this pull request as ready for review March 7, 2024 05:27
@@ -473,7 +474,7 @@ func (t *Transactor) BuildConfirmBatchTxn(ctx context.Context, batchHeader *core
QuorumThresholdPercentages: quorumThresholdPercentages,
ReferenceBlockNumber: uint32(batchHeader.ReferenceBlockNumber),
}
t.Logger.Trace("[ConfirmBatch] batch header", "batchHeaderReferenceBlock", batchH.ReferenceBlockNumber, "batchHeaderRoot", gethcommon.Bytes2Hex(batchH.BlobHeadersRoot[:]), "quorumNumbers", gethcommon.Bytes2Hex(batchH.QuorumNumbers), "quorumThresholdPercentages", gethcommon.Bytes2Hex(batchH.QuorumThresholdPercentages))
t.Logger.Debug("[ConfirmBatch] batch header", "batchHeaderReferenceBlock", batchH.ReferenceBlockNumber, "batchHeaderRoot", gethcommon.Bytes2Hex(batchH.BlobHeadersRoot[:]), "quorumNumbers", gethcommon.Bytes2Hex(batchH.QuorumNumbers), "quorumThresholdPercentages", gethcommon.Bytes2Hex(batchH.QuorumThresholdPercentages))
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioned this before but I feel instead of doing "[Method/Component]" in the message we should just add it as a key/value pair.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe separate PR since this one is already really big.

disperser/batcher/batcher.go Show resolved Hide resolved
node/cmd/main.go Outdated Show resolved Hide resolved
@ian-shim ian-shim force-pushed the replace-logger branch 2 times, most recently from ec4b38c to 1557804 Compare March 7, 2024 23:33
@ian-shim ian-shim requested a review from dmanc March 7, 2024 23:43
Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for making this big upgrade

common/logger_config.go Outdated Show resolved Hide resolved
common/logger_config.go Outdated Show resolved Hide resolved
@ian-shim ian-shim requested a review from jianoaix March 8, 2024 19:25
// but we might eventually want to move as much as possible to the sdk
//
//lint:ignore U1000 this function will be used once we move to the sdk
func buildSdkClients(config *Config, logger common.Logger) (*constructor.Clients, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this just directly using SDK now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should. But also, this method is not being used anywhere.
I think it should be added when it's actually needed.

@ian-shim ian-shim merged commit 29b2bec into Layr-Labs:master Mar 8, 2024
4 checks passed
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