From 390e491f7db9b5278be2953ce81c2eb7fb7081b0 Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Thu, 31 Oct 2024 15:12:34 +0000 Subject: [PATCH] fix(e2e tests): missing new eigenda-client required config fields (#196) * fix(e2e tests): missing new eigenda-client required config fields - ethrpc and svcmanageraddr * Revert "ci: give holesky-test workflow access to secrets via pull_request_target (#153)" This reverts commit 15b10fd0feaa324ae49c143cb642d1b3e39ffc26. The commit was doing things very wrong. I hadn't understood how pull_request_target works. Was causing the workflow to run against main branch head commit instead of PR commit. We will need to find another solution to the problem of letting external contributors run this workflow. * ide: update vscode settings.json with env vars to run holesky testnet e2e tests * docs: shorten svcManagerAddr holesky testnet comment * tests: add panic when both INTEGRATION and TESTNET env vars are set This forces test runner to be fully aware of which test suite he is running (otherwise it implicitly runs the TESTNET suite) * style: make handleGetShared returned error print hex encoded commitment Previously it was printing as byte array, which is unreadable and clutters logs * docs: better comments in metrics middleware * deps: upgrade eigenda dep to regression fix commit TODO: will need to update this after that PR is merged * deps: update eigenda dependency to master head Just merged https://github.com/Layr-Labs/eigenda/pull/849 which will fix our ci bug --- .github/workflows/holesky-test.yml | 6 +----- .vscode/settings.json | 7 ++++++- e2e/main_test.go | 3 +++ e2e/setup.go | 5 ++++- go.mod | 2 +- go.sum | 4 ++-- server/handlers.go | 5 +++-- server/middleware.go | 4 +++- 8 files changed, 23 insertions(+), 13 deletions(-) diff --git a/.github/workflows/holesky-test.yml b/.github/workflows/holesky-test.yml index 34b3e7be..6ae3743f 100644 --- a/.github/workflows/holesky-test.yml +++ b/.github/workflows/holesky-test.yml @@ -3,11 +3,7 @@ name: holesky-test on: push: branches: [ "main" ] - # pull_request_target is needed so that external contributors that create PRs from a forked repo - # have access to the secrets needed to run the tests. There are security implications to this, - # see https://stackoverflow.com/questions/74957218/what-is-the-difference-between-pull-request-and-pull-request-target-event-in-git - # MAKE SURE TO ONLY ALLOW RUNNING THIS WORKFLOW AFTER REVIEWING THE PR! - pull_request_target: + pull_request: branches: [ "main" ] jobs: diff --git a/.vscode/settings.json b/.vscode/settings.json index 3d2eb80e..736563a9 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,6 +1,11 @@ { "go.testEnvVars": { - "INTEGRATION": "true" + // Only one of INTEGRATION or TESTNET should be true. + "INTEGRATION": "true", + "TESTNET": "false", + // To test against TESTNET, set SIGNER_PRIVATE_KEY and ETHEREUM_RPC. + "SIGNER_PRIVATE_KEY": "", + "ETHEREUM_RPC": "https://ethereum-holesky-rpc.publicnode.com" }, "go.testFlags": [ "-test.parallel", diff --git a/e2e/main_test.go b/e2e/main_test.go index 39a2c062..d0639fa9 100644 --- a/e2e/main_test.go +++ b/e2e/main_test.go @@ -30,6 +30,9 @@ var ( func ParseEnv() { runIntegrationTests = os.Getenv("INTEGRATION") == "true" || os.Getenv("INTEGRATION") == "1" runTestnetIntegrationTests = os.Getenv("TESTNET") == "true" || os.Getenv("TESTNET") == "1" + if runIntegrationTests && runTestnetIntegrationTests { + panic("only one of INTEGRATION=true or TESTNET=true env var can be set") + } } // TestMain ... run main controller diff --git a/e2e/setup.go b/e2e/setup.go index fe4aefa1..5c8e3864 100644 --- a/e2e/setup.go +++ b/e2e/setup.go @@ -183,6 +183,7 @@ func TestSuiteConfig(testCfg *Cfg) server.CLIConfig { panic(err) } + svcManagerAddr := "0xD4A7E1Bd8015057293f0D0A557088c286942e84b" // holesky testnet eigendaCfg := server.Config{ EdaClientConfig: clients.EigenDAClientConfig{ RPC: holeskyDA, @@ -190,11 +191,13 @@ func TestSuiteConfig(testCfg *Cfg) server.CLIConfig { StatusQueryRetryInterval: pollInterval, DisableTLS: false, SignerPrivateKeyHex: pk, + EthRpcUrl: ethRPC, + SvcManagerAddr: svcManagerAddr, }, VerifierConfig: verify.Config{ VerifyCerts: false, RPCURL: ethRPC, - SvcManagerAddr: "0xD4A7E1Bd8015057293f0D0A557088c286942e84b", // incompatible with non holeskly networks + SvcManagerAddr: svcManagerAddr, EthConfirmationDepth: 0, KzgConfig: &kzg.KzgConfig{ G1Path: "../resources/g1.point", diff --git a/go.mod b/go.mod index d012c3a0..38e89c39 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.22 toolchain go1.22.0 require ( - github.com/Layr-Labs/eigenda v0.8.5-0.20241028201743-5fe3e910a22d + github.com/Layr-Labs/eigenda v0.8.5-0.20241031144746-e2ead56a306d github.com/consensys/gnark-crypto v0.12.1 github.com/ethereum-optimism/optimism v1.9.4-0.20240927020138-a9c7f349d10b github.com/ethereum/go-ethereum v1.14.11 diff --git a/go.sum b/go.sum index e3eb27f3..8e602d79 100644 --- a/go.sum +++ b/go.sum @@ -22,8 +22,8 @@ github.com/BurntSushi/toml v1.4.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2 github.com/DataDog/datadog-go v3.2.0+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ= github.com/DataDog/zstd v1.5.6-0.20230824185856-869dae002e5e h1:ZIWapoIRN1VqT8GR8jAwb1Ie9GyehWjVcGh32Y2MznE= github.com/DataDog/zstd v1.5.6-0.20230824185856-869dae002e5e/go.mod h1:g4AWEaM3yOg3HYfnJ3YIawPnVdXJh9QME85blwSAmyw= -github.com/Layr-Labs/eigenda v0.8.5-0.20241028201743-5fe3e910a22d h1:s5U1TaWhC1J2rwc9IQdU/COGvuXALCKMd9GbONUZMxc= -github.com/Layr-Labs/eigenda v0.8.5-0.20241028201743-5fe3e910a22d/go.mod h1:sqUNf9Ak+EfAX82jDxrb4QbT/g3DViWD3b7YIk36skk= +github.com/Layr-Labs/eigenda v0.8.5-0.20241031144746-e2ead56a306d h1:2JtVArkLjW61kilkvvLyFHXBMp0ClF8PYCAxWqRnoDQ= +github.com/Layr-Labs/eigenda v0.8.5-0.20241031144746-e2ead56a306d/go.mod h1:sqUNf9Ak+EfAX82jDxrb4QbT/g3DViWD3b7YIk36skk= github.com/Layr-Labs/eigensdk-go v0.1.7-0.20240507215523-7e4891d5099a h1:L/UsJFw9M31FD/WgXTPFB0oxbq9Cu4Urea1xWPMQS7Y= github.com/Layr-Labs/eigensdk-go v0.1.7-0.20240507215523-7e4891d5099a/go.mod h1:OF9lmS/57MKxS0xpSpX0qHZl0SKkDRpvJIvsGvMN1y8= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= diff --git a/server/handlers.go b/server/handlers.go index b9877cfb..ea04872b 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -95,11 +95,12 @@ func (svr *Server) handleGetOPGenericCommitment(w http.ResponseWriter, r *http.R } func (svr *Server) handleGetShared(ctx context.Context, w http.ResponseWriter, comm []byte, meta commitments.CommitmentMeta) error { - svr.log.Info("Processing GET request", "commitment", hex.EncodeToString(comm), "commitmentMeta", meta) + commitmentHex := hex.EncodeToString(comm) + svr.log.Info("Processing GET request", "commitment", commitmentHex, "commitmentMeta", meta) input, err := svr.sm.Get(ctx, comm, meta.Mode) if err != nil { err = MetaError{ - Err: fmt.Errorf("get request failed with commitment %v: %w", comm, err), + Err: fmt.Errorf("get request failed with commitment %v: %w", commitmentHex, err), Meta: meta, } if errors.Is(err, ErrNotFound) { diff --git a/server/middleware.go b/server/middleware.go index 19b87de2..65f389bc 100644 --- a/server/middleware.go +++ b/server/middleware.go @@ -12,6 +12,7 @@ import ( ) // withMetrics is a middleware that records metrics for the route path. +// It does not write anything to the response, that is the job of the handlers. func withMetrics( handleFn func(http.ResponseWriter, *http.Request) error, m metrics.Metricer, @@ -30,9 +31,10 @@ func withMetrics( } return err } - // we assume that every route will set the status header + // TODO: some routes don't have a version byte, such as /put simple commitments versionByte, err := parseVersionByte(w, r) if err != nil { + // we assume that every route will set the status header recordDur(w.Header().Get("status"), string(mode), string(versionByte)) return fmt.Errorf("metrics middleware: error parsing version byte: %w", err) }