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

Component updates for M2 mainnet contracts #196

Conversation

mooselumph
Copy link
Collaborator

Why are these changes needed?

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 :(

@@ -47,7 +47,7 @@ type churner struct {
mu sync.Mutex
Indexer thegraph.IndexedChainState
Transactor core.Transactor
QuorumCount uint16
QuorumCount uint8
Copy link
Contributor

Choose a reason for hiding this comment

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

The max of uint8 is 255, so it can have 256 quorums, and uint8 cannot represent 256.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The onchain quorumCount was changed to a uint8 (https://github.com/Layr-Labs/eigenlayer-middleware/blob/98f884454d9e9de1e344bb6fba9a2cd3915e5b57/src/RegistryCoordinator.sol#L71), so it looks like we would only support up to 255 quorums.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so the range of quorum ID is [0, 254] (not [0, 255]).

// if we should call the churner, call it
if shouldCallChurner {
churnReply, err := requestChurnApproval(ctx, operator, churnerUrl, useSecureGrpc, logger)
if err != nil {
return fmt.Errorf("failed to request churn approval: %w", err)
}

return transactor.RegisterOperatorWithChurn(ctx, operator.KeyPair.PubKey, operator.Socket, operator.QuorumIDs, churnReply)
return transactor.RegisterOperatorWithChurn(ctx, operator.KeyPair, operator.Socket, operator.QuorumIDs, operator.PrivKey, salt, expiry, churnReply)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to update the nodeplugin path to have this private key as well: https://github.com/Layr-Labs/eigenda/blob/master/node/plugin/cmd/main.go#L118

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Updated!

disperser/batcher/batcher.go Outdated Show resolved Hide resolved
}
metrics := NewMetrics(sdkClients.Metrics, sdkClients.PrometheusRegistry, logger, ":"+config.MetricsPort)
rpcCallsCollector := rpccalls.NewCollector(AppName, sdkClients.PrometheusRegistry)
// sdkClients, err := buildSdkClients(config, logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At the time, the SDK hadn't been updated to reflect the new contracts. Since the client from the SDK was only being used for metrics, I created the metrics separately.

I'm not sure if it makes sense to build all of the SDK clients if we aren't using them.

My preference here would be to remove this commented code and create a ticket to track further integration with the SDK.

@@ -47,7 +47,7 @@ type churner struct {
mu sync.Mutex
Indexer thegraph.IndexedChainState
Transactor core.Transactor
QuorumCount uint16
QuorumCount uint8
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so the range of quorum ID is [0, 254] (not [0, 255]).

@mooselumph
Copy link
Collaborator Author

As this has one approval, I'm merging for the stake of unblocking. Will continue to monitor comments and create additional PRs as needed.

@mooselumph mooselumph merged commit aa7b912 into Layr-Labs:m2-mainnet-contracts Jan 25, 2024
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