Skip to content

Conversation

ethenotethan
Copy link
Contributor

@ethenotethan ethenotethan commented Jun 6, 2025

Currently debug logs are always being presented in proxy for NTP clock synchronization updates. This should instead use a preconfigured logger passed through via dependency injection

Why are these changes needed?

Checks

  • 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.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@Copilot Copilot AI review requested due to automatic review settings June 6, 2025 19:42
@ethenotethan ethenotethan requested a review from a team as a code owner June 6, 2025 19:42
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the Disperser client to accept an external logger instead of creating its own, and updates all call sites (tests, examples, and integration setup) to pass in a logger.

  • Change signature of NewDisperserClient to take a logging.Logger
  • Update tests and integration code to construct and pass a common.NewLogger
  • Adjust example usage in client_construction.go to include a logger parameter

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/v2/correctness/correctness_test.go Added import of common, instantiated logger, and passed it into NewDisperserClient
test/v2/client/test_client.go Propagated logger to NewDisperserClient in test client setup
inabox/tests/integration_suite_test.go Updated integration setup to pass logger into the new client constructor
api/clients/v2/examples/client_construction.go Modified example to call createDisperserClient(logger, …), updated its signature
api/clients/v2/disperser_client.go Changed NewDisperserClient signature, removed internal logger init, wired logger.With into NTP clock
Comments suppressed due to low confidence (2)

api/clients/v2/examples/client_construction.go:50

  • The variable logger is undefined in createPayloadDisperser. Either initialize a logger before calling createDisperserClient or change createPayloadDisperser to accept a logger parameter.
disperserClient, err := createDisperserClient(logger, privateKey, kzgProver)

api/clients/v2/disperser_client.go:88

  • [nitpick] Consider validating that the provided logger is non-nil at the start of the constructor to fail fast and avoid panics when calling logger.With later.
func NewDisperserClient(logger logging.Logger, config *DisperserClientConfig, signer corev2.BlobRequestSigner, prover encoding.Prover, accountant *Accountant) (*disperserClient, error) {

@ethenotethan ethenotethan changed the title fix(clients_v2): Update disperser client to take in logger vs initializing default one for ntp clock fix: Update disperser client to take in logger vs initializing default one for ntp clock Jun 6, 2025
@ethenotethan ethenotethan requested review from hopeyen and litt3 June 6, 2025 19:55
@ethenotethan ethenotethan merged commit 7f93aea into master Jun 8, 2025
12 checks passed
@ethenotethan ethenotethan deleted the ethenotethan--fix-update-disperser-client-logging branch June 8, 2025 20:25
samlaf added a commit that referenced this pull request Jul 12, 2025
…g default one for ntp clock (#1651)"

This reverts commit 7f93aea.
samlaf added a commit that referenced this pull request Jul 16, 2025
* Revert "fix: Update disperser client to take in logger vs initializing default one for ntp clock (#1651)"

This reverts commit 7f93aea.

* Revert "refactor: ntp init logs but not panic (#1568)"

This reverts commit 00256fd.

* Revert "feat: validator/disperser clock synchrony (#1509)"

This reverts commit 5c19ace.

* fix: NewDispserserClient call

Remove no longer needed log first argument

* chore: go mod tidy
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