feat(faucet-app): migrate faucet-app into nitrolite repo#776
Open
nksazonov wants to merge 5 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete Go-based token faucet server with Nitronode SDK integration. It adds configuration management, structured logging, an HTTP REST API with rate limiting, graceful shutdown handling, comprehensive tests, Docker containerization, and full documentation. ChangesToken Faucet Server Implementation
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faucet-app/server/.env.example (1)
17-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
.env.exampleis out of sync with runtime config keys.Line 29 uses
CLEARNODE_URLwhileConfigexpectsNITRONODE_URL, and requiredCOOLDOWN_PERIODis missing.SIGNER_PRIVATE_KEYis also not used byConfig. This will break startup for users copying this template.Suggested fix
-# Clearnode Configuration +# Nitronode Configuration @@ -# REQUIRED: Used for EIP-712 authentication with Clearnode +# REQUIRED: Used for EIP-712 authentication with Nitronode OWNER_PRIVATE_KEY=your_owner_private_key_here_without_0x_prefix -# Private key for transaction signing (without 0x prefix) -# REQUIRED: Used for signing transfer transactions - must be different from OWNER_PRIVATE_KEY -SIGNER_PRIVATE_KEY=your_signer_private_key_here_without_0x_prefix - -# Clearnode WebSocket URL -# REQUIRED: The WebSocket endpoint for your Clearnode instance -CLEARNODE_URL=wss://clearnode.example.com/ws +# Nitronode WebSocket URL +# REQUIRED: The WebSocket endpoint for your Nitronode instance +NITRONODE_URL=wss://nitronode.example.com/ws @@ -# REQUIRED: The symbol of the token to distribute (must be supported by Clearnode) +# REQUIRED: The symbol of the token to distribute (must be supported by Nitronode) TOKEN_SYMBOL=usdc @@ MIN_TRANSFER_COUNT=5 + +# Cooldown between requests per wallet/IP +# REQUIRED: Duration (e.g. 24h, 1h30m) +COOLDOWN_PERIOD=24h🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/.env.example` around lines 17 - 49, The .env.example keys are out of sync with the runtime Config: update the file so environment variable names and required keys match the Config object—replace CLEARNODE_URL with NITRONODE_URL (used by Config), add the missing COOLDOWN_PERIOD entry with a short description and required format, and remove or mark SIGNER_PRIVATE_KEY as unused (since Config does not reference it) so users copying the template won't cause startup failures; reference the Config symbol and the environment keys CLEARNODE_URL, NITRONODE_URL, COOLDOWN_PERIOD, and SIGNER_PRIVATE_KEY when making the edits.
🧹 Nitpick comments (6)
faucet-app/scripts/auto_tag.sh (1)
1-23: ⚡ Quick winAdd fail-fast shell options to avoid cascading tag/push failures.
If
git tagfails (e.g., concurrent run/tag exists), the script should stop immediately.Suggested fix
#!/bin/sh +set -eu🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/scripts/auto_tag.sh` around lines 1 - 23, The script can continue after failures (e.g., concurrent tag) so add fail-fast shell options at the top (enable set -euo pipefail or equivalent) to cause immediate exit on errors/unset vars and ensure pipelines fail fast; also ensure the critical commands git tag and git push are not masked by conditional substitutions—leave git tag "$new_rc_tag" and git push origin "$new_rc_tag" as-is so they will abort the script on failure, and verify variables last_rc_tag, last_stable_tag and new_rc_tag are referenced after enabling -u to catch unset usage.faucet-app/server/internal/nitronode/client.go (1)
46-46: ⚡ Quick winAdd doc comment for exported constructor.
NewClientis exported and needs a doc comment.As per coding guidelines,
**/*.go: Followgofmtformatting and include doc comments on all exported names in Go code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/internal/nitronode/client.go` at line 46, Add a Go doc comment immediately above the exported constructor NewClient that begins with "NewClient" and briefly explains what the function constructs/returns, its important parameters (privateKeyHex, nitronodeURL, tokenSymbol, tipAmount, minTransferCount) and the return values (*Client, error); place the comment directly above the func NewClient declaration so it follows Go doc conventions and gofmt formatting.faucet-app/server/internal/logger/logger.go (1)
9-68: ⚡ Quick winDocument exported logger symbols.
Exported
Log,Initialize, and all exported wrapper funcs need doc comments.As per coding guidelines,
**/*.go: Followgofmtformatting and include doc comments on all exported names in Go code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/internal/logger/logger.go` around lines 9 - 68, Add Go doc comments for the exported symbols to satisfy the project's linting/gofmt rules: add a package-level comment if missing, then add brief one-line comments above the exported Log variable and the exported functions Initialize, Info, Infof, Warn, Warnf, Error, Errorf, Debug, Debugf, Fatal, and Fatalf describing their purpose and behavior (e.g., "Log is the package logger.", "Initialize sets up the package logger with the given level.", "Info logs an info message."), and run gofmt/golangci-lint to ensure formatting and comment style conform to guidelines.faucet-app/server/internal/config/config.go (1)
11-46: ⚡ Quick winAdd Go doc comments to exported names.
Config,Load, andValidateare exported and need doc comments.As per coding guidelines,
**/*.go: Followgofmtformatting and include doc comments on all exported names in Go code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/internal/config/config.go` around lines 11 - 46, Add proper Go doc comments for the exported symbols: place a full-sentence comment immediately above the Config type, the Load function, and the (c *Config) Validate method describing their purpose and behavior (start the sentence with the symbol name, e.g., "Config ...", "Load ...", "Validate ..."). Ensure the comments mention any important semantics (e.g., Load reads .env/env and returns a validated Config, Validate checks/parses fields like StandardTipAmountDecimal and CooldownPeriodDuration) and run gofmt/golangci-lint if needed to keep formatting consistent.faucet-app/server/internal/server/server.go (1)
35-57: ⚡ Quick winAdd doc comments to all exported types/functions.
Server,FaucetRequest,FaucetResponse,ErrorResponse,NewServer, andStartare exported but undocumented (Line 35, Line 42, Line 46, Line 55, Line 59, Line 198). Please add Go doc comments for each exported name.As per coding guidelines, "Follow
gofmtformatting and add doc comments on all exported names in Go code".Also applies to: 59-59, 198-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/internal/server/server.go` around lines 35 - 57, Add Go doc comments for each exported symbol: Server, FaucetRequest, FaucetResponse, ErrorResponse, NewServer, and Start. For types (Server, FaucetRequest, FaucetResponse, ErrorResponse) add a one-line comment immediately above the type that begins with the type name and briefly states its purpose (e.g., "Server manages ...", "FaucetRequest represents ..."). For functions NewServer and Start add comments that begin with the function name and describe what they do and the meaning of their parameters/returns. Place comments directly above the corresponding declarations so they follow godoc conventions.faucet-app/server/internal/server/server_test.go (1)
70-70: ⚡ Quick winDo not ignore test setup/marshal errors with
_.These lines ignore returned errors (
_) fromlogger.Initializeandjson.Marshal. Please assert/require these errors explicitly so test failures are deterministic.Suggested pattern
- body, _ := json.Marshal(FaucetRequest{UserAddress: testAddress}) + body, err := json.Marshal(FaucetRequest{UserAddress: testAddress}) + require.NoError(t, err)- _ = logger.Initialize("debug") + if err := logger.Initialize("debug"); err != nil { + panic(err) + }As per coding guidelines, "Always check and return errors in Go; never ignore with
_".Also applies to: 79-79, 106-106, 122-122, 140-140, 158-158, 177-177, 199-199, 227-227, 251-252, 276-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/internal/server/server_test.go` at line 70, Replace ignored error returns in the tests with explicit assertions: for calls like logger.Initialize(...) and json.Marshal(...) (and other occurrences listed) capture the returned error and assert/require it's nil (e.g., err := logger.Initialize("debug"); require.NoError(t, err) or similar) so test setup failures fail deterministically; do the same for json.Marshal results (data, err := json.Marshal(...); require.NoError(t, err)) and any other places where `_ =` was used to discard errors (use the test's assert/require helpers used elsewhere in server_test.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@faucet-app/.github/dependabot.yml`:
- Around line 8-10: Dependabot config uses package-ecosystem: "gomod" but the
directory is still set to "/server/" which no longer matches the migrated module
layout; update the directory value in .github/dependabot.yml to the repository
root or the actual Go module directory for this faucet-app (replace the current
"/server/" value with "/" or the correct module path) so Dependabot runs gomod
updates against the proper module.
In `@faucet-app/.github/workflows/main-pr.yml`:
- Around line 19-25: Update the GitHub Actions workflow to use the repository's
post-migration paths: change occurrences referencing the old root "server" to
"faucet-app/server" (e.g., go-version-file: 'server/go.mod',
cache-dependency-path: 'server/go.sum', and working-directory: server used in
the "Test" step). Also apply the same replacement for the duplicate occurrences
noted around lines 52-53 so all workflow steps (setup, cache, test, build) point
to faucet-app/server instead of server.
In `@faucet-app/.github/workflows/main-push.yml`:
- Around line 19-25: The workflow uses root-relative paths that still include a
removed top-level "server/" prefix (e.g. go-version-file: 'server/go.mod',
cache-dependency-path: 'server/go.sum', working-directory: server), so update
all occurrences to the repository's current layout by removing the obsolete
"server/" prefix: set go-version-file to 'go.mod', cache-dependency-path to
'go.sum' (or other correct relative file), and change working-directory entries
(e.g. under the "Test" step and any build/tag/deploy steps) to '.' or remove the
working-directory so actions run from the repo root; apply the same replacement
to the other mentioned occurrences (lines ~48, ~91-92, ~155-159) to ensure all
steps reference the correct files and directories.
In `@faucet-app/.github/workflows/stable-tag.yml`:
- Around line 41-42: The workflow references migrated paths that no longer
resolve: replace the literal "context: server" with the repo-relative build
context (e.g., "context: ./server" or the actual relative directory that
contains the Dockerfile) and update every "chart/..." reference to the new
Helm/chart location in the repository (use the exact new relative path to the
chart directory); keep "push: true" as-is and apply the same fixes to the other
occurrences (the block around the later chart references mentioned) so all steps
use the correct migrated paths.
- Line 6: The on.push.tags glob 'v[0-9]+.[0-9]+.[0-9]+' won't match SemVer tags
in GitHub Actions; replace the tag matcher with a permissive glob like 'v*' or
'v*.*.*' in stable-tag.yml (change the value under on.push.tags) and add a
validation step in your workflow job that checks the tag string (use
github.ref_name or GITHUB_REF) with a proper SemVer regex before proceeding so
only true SemVer tags trigger the release steps.
In `@faucet-app/chart/README.md.gotmpl`:
- Line 20: Update the helm install source URL that still points to
"git+https://github.com/erc7824/clearnode@chart?ref=main" so it references the
migrated faucet chart in the nitrolite repo/path; replace that repository path
in the helm command(s) (the "helm install my-release ..." lines found in the
template) with the correct git URL for the nitrolite faucet chart (e.g., the
equivalent "git+https://github.com/nitrolite/<faucet-repo>`@chart`?ref=main") for
all occurrences (including the duplicate at the other noted line).
- Around line 88-90: The README uses a stale selector `-l app=clearnode`; update
the troubleshooting command to use the chart's actual pod labels used in the
templates (for example the standard chart labels such as
`app.kubernetes.io/name=<chart-name>` or the release-specific label rendered by
the template like `app: {{ include "faucet-app.name" . }}`). Replace `kubectl
logs -l app=clearnode` with a selector that matches the Deployment/Pod labels
defined in the chart templates (e.g. `kubectl logs -l
app.kubernetes.io/name=faucet-app` or the exact label key/value used in your
templates) so the command reliably finds pods.
In `@faucet-app/chart/templates/helpers/_common.tpl`:
- Around line 73-75: The environment mapping uses stale keys: replace
.Values.service.http.internalPort with the actual key .Values.service.http.port
and update the clearnode env var to match the values file by renaming
CLEARNODE_URL to NITRONODE_URL and using .Values.config.nitronodeWsUrl (instead
of .Values.config.clearnodeWsUrl) in the template so the env var name and value
key align with the provided values.
In `@faucet-app/chart/templates/helpers/_component.tpl`:
- Around line 45-54: The resources helper ("faucet-app.component.resources")
currently sources limit values and ephemeral-storage from .requests, causing
incorrect mappings; update the template so requests use .requests.cpu,
.requests.memory, and .requests.ephemeral-storage with their defaults, and
limits use .limits.cpu, .limits.memory, and .limits.ephemeral-storage with their
defaults (keeping the existing fallback values like "100m", "128Mi", "100Mi");
specifically change the two limits lines that reference .requests.* to reference
.limits.* and change ephemeral-storage lines to read the correct
.requests.ephemeral-storage and .limits.ephemeral-storage fields.
In `@faucet-app/server/internal/config/config.go`:
- Around line 46-67: The Validate method is missing a range check for
Config.MinTransferCount; update func (c *Config) Validate() to return an error
when c.MinTransferCount is zero or negative (e.g. fmt.Errorf("MIN_TRANSFER_COUNT
must be a positive integer")), so the minimum-balance guard remains effective;
add this check (and error message) alongside the other validations in Validate,
referencing the MinTransferCount field and the Validate method.
- Around line 32-37: The Load function should only fall back to cleanenv.ReadEnv
when the .env file is missing—change the error handling in Load to detect
file-missing errors using os.IsNotExist(err) or errors.As(err, *os.PathError)
and return the original cleanenv.ReadConfig error for any other parse/IO errors;
update references to the Load function and the Config type accordingly. In
(*Config).Validate ensure MinTransferCount is range-validated (e.g., > 0) and
return a clear validation error from Validate when out of range. Add concise Go
doc comments for the exported Config type, Load function, and (*Config).Validate
method. Finally, refactor the Config construction to follow the
functional-options pattern used elsewhere (e.g., provide a NewConfig or Load
accepting variadic option functions) so this file aligns with sdk/go/config.go
conventions.
In `@faucet-app/server/internal/nitronode/client_validation_test.go`:
- Line 34: The test currently discards both results from NewClient; change the
call to capture the values (e.g., client, err := NewClient(...)) and assert the
expected outcome instead of using `_`; for example, check err is nil (using
t.Fatalf/t.Fatal or your test helper like require.NoError) and that client is
non-nil (or validate its fields) so the test fails on unexpected errors from
NewClient; reference the NewClient function and the test function where the call
occurs to locate and update the assertion logic.
In `@faucet-app/server/internal/nitronode/client.go`:
- Around line 205-210: validateTokenSupport can set c.tokenSupported based on an
old sdkClient after a reconnect; to fix, capture the current client into a local
variable at the top of validateTokenSupport (e.g., local := c.sdkClient), use
that local for all calls and comparisons, and only update c.tokenSupported
(under c.mu) if c.sdkClient is still equal to that captured local; ensure any
early returns that set tokenSupported perform the same check so the cache isn't
updated for a replaced client.
- Line 160: The call to newClient.Close() currently ignores its error; change it
to capture and handle the error (e.g. err := newClient.Close()) and then either
return the wrapped error or log it with context instead of discarding it. Update
the code path that calls newClient.Close() (reference symbol: newClient.Close)
to check err and propagate or log a descriptive message (wrap with fmt.Errorf or
use the existing logger) so cleanup failures are not silently ignored.
In `@faucet-app/server/internal/server/server.go`:
- Around line 198-202: Change Server.Start to accept a context.Context and stop
using s.router.Run; instead create an http.Server with Addr set to ":" +
s.config.ServerPort and Handler set to s.router, start it in a goroutine
(server.ListenAndServe), then block on ctx.Done and call server.Shutdown(ctx)
(with a short timeout context if you want deadline control) to perform graceful
shutdown. Update the function signature Server.Start(ctx context.Context) and
ensure errors from ListenAndServe and Shutdown are returned (or logged) so
callers can observe startup/shutdown failures; reference Server.Start,
s.router.Run, s.config.ServerPort, and server.Shutdown in your changes.
In `@faucet-app/server/main.go`:
- Around line 28-29: The current logger.Infof call prints cfg.NitronodeURL
verbatim which may leak embedded credentials; update the logging in main.go
where logger.Infof is called to parse cfg.NitronodeURL (e.g., using url.Parse)
and log only safe parts such as Scheme and Host or replace userinfo with a
redacted placeholder before passing to logger.Infof, keeping cfg.ServerPort as
before and ensuring the log never outputs cfg.NitronodeURL raw.
In `@faucet-app/server/README.md`:
- Around line 20-25: Update README references from the old clearnode names to
the migrated nitronode names: replace any mention of the package
`internal/clearnode` with `internal/nitronode` and change the environment
variable `CLEARNODE_URL` to `NITRONODE_URL` (and any other CLEARNODE_* vars) so
docs match the codebase; ensure the “Clearnode Client” heading and explanatory
text are renamed to “Nitronode Client” and that the description refers to the
Nitrolite SDK `sdk.Client` usage under `internal/nitronode`.
---
Outside diff comments:
In `@faucet-app/server/.env.example`:
- Around line 17-49: The .env.example keys are out of sync with the runtime
Config: update the file so environment variable names and required keys match
the Config object—replace CLEARNODE_URL with NITRONODE_URL (used by Config), add
the missing COOLDOWN_PERIOD entry with a short description and required format,
and remove or mark SIGNER_PRIVATE_KEY as unused (since Config does not reference
it) so users copying the template won't cause startup failures; reference the
Config symbol and the environment keys CLEARNODE_URL, NITRONODE_URL,
COOLDOWN_PERIOD, and SIGNER_PRIVATE_KEY when making the edits.
---
Nitpick comments:
In `@faucet-app/scripts/auto_tag.sh`:
- Around line 1-23: The script can continue after failures (e.g., concurrent
tag) so add fail-fast shell options at the top (enable set -euo pipefail or
equivalent) to cause immediate exit on errors/unset vars and ensure pipelines
fail fast; also ensure the critical commands git tag and git push are not masked
by conditional substitutions—leave git tag "$new_rc_tag" and git push origin
"$new_rc_tag" as-is so they will abort the script on failure, and verify
variables last_rc_tag, last_stable_tag and new_rc_tag are referenced after
enabling -u to catch unset usage.
In `@faucet-app/server/internal/config/config.go`:
- Around line 11-46: Add proper Go doc comments for the exported symbols: place
a full-sentence comment immediately above the Config type, the Load function,
and the (c *Config) Validate method describing their purpose and behavior (start
the sentence with the symbol name, e.g., "Config ...", "Load ...", "Validate
..."). Ensure the comments mention any important semantics (e.g., Load reads
.env/env and returns a validated Config, Validate checks/parses fields like
StandardTipAmountDecimal and CooldownPeriodDuration) and run gofmt/golangci-lint
if needed to keep formatting consistent.
In `@faucet-app/server/internal/logger/logger.go`:
- Around line 9-68: Add Go doc comments for the exported symbols to satisfy the
project's linting/gofmt rules: add a package-level comment if missing, then add
brief one-line comments above the exported Log variable and the exported
functions Initialize, Info, Infof, Warn, Warnf, Error, Errorf, Debug, Debugf,
Fatal, and Fatalf describing their purpose and behavior (e.g., "Log is the
package logger.", "Initialize sets up the package logger with the given level.",
"Info logs an info message."), and run gofmt/golangci-lint to ensure formatting
and comment style conform to guidelines.
In `@faucet-app/server/internal/nitronode/client.go`:
- Line 46: Add a Go doc comment immediately above the exported constructor
NewClient that begins with "NewClient" and briefly explains what the function
constructs/returns, its important parameters (privateKeyHex, nitronodeURL,
tokenSymbol, tipAmount, minTransferCount) and the return values (*Client,
error); place the comment directly above the func NewClient declaration so it
follows Go doc conventions and gofmt formatting.
In `@faucet-app/server/internal/server/server_test.go`:
- Line 70: Replace ignored error returns in the tests with explicit assertions:
for calls like logger.Initialize(...) and json.Marshal(...) (and other
occurrences listed) capture the returned error and assert/require it's nil
(e.g., err := logger.Initialize("debug"); require.NoError(t, err) or similar) so
test setup failures fail deterministically; do the same for json.Marshal results
(data, err := json.Marshal(...); require.NoError(t, err)) and any other places
where `_ =` was used to discard errors (use the test's assert/require helpers
used elsewhere in server_test.go).
In `@faucet-app/server/internal/server/server.go`:
- Around line 35-57: Add Go doc comments for each exported symbol: Server,
FaucetRequest, FaucetResponse, ErrorResponse, NewServer, and Start. For types
(Server, FaucetRequest, FaucetResponse, ErrorResponse) add a one-line comment
immediately above the type that begins with the type name and briefly states its
purpose (e.g., "Server manages ...", "FaucetRequest represents ..."). For
functions NewServer and Start add comments that begin with the function name and
describe what they do and the meaning of their parameters/returns. Place
comments directly above the corresponding declarations so they follow godoc
conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2079deb1-ba16-4650-b36e-addac1962676
⛔ Files ignored due to path filters (1)
faucet-app/server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (40)
faucet-app/.github/CODEOWNERSfaucet-app/.github/dependabot.ymlfaucet-app/.github/workflows/main-pr.ymlfaucet-app/.github/workflows/main-push.ymlfaucet-app/.github/workflows/stable-tag.ymlfaucet-app/LICENSEfaucet-app/chart/.helmignorefaucet-app/chart/Chart.yamlfaucet-app/chart/README.mdfaucet-app/chart/README.md.gotmplfaucet-app/chart/config/sandbox/secrets.yamlfaucet-app/chart/config/sandbox/values.yamlfaucet-app/chart/config/uat/secrets.yamlfaucet-app/chart/config/uat/values.yamlfaucet-app/chart/templates/deployment.yamlfaucet-app/chart/templates/gateway.yamlfaucet-app/chart/templates/helpers/_common.tplfaucet-app/chart/templates/helpers/_component.tplfaucet-app/chart/templates/helpers/_hpa.tplfaucet-app/chart/templates/helpers/_ingress.tplfaucet-app/chart/templates/http-route-redirect.yamlfaucet-app/chart/templates/http-route.yamlfaucet-app/chart/templates/ingress.yamlfaucet-app/chart/templates/podmonitoring.yamlfaucet-app/chart/templates/service.yamlfaucet-app/chart/values.yamlfaucet-app/scripts/auto_tag.shfaucet-app/server/.env.examplefaucet-app/server/.gitignorefaucet-app/server/Dockerfilefaucet-app/server/README.mdfaucet-app/server/go.modfaucet-app/server/internal/config/config.gofaucet-app/server/internal/logger/logger.gofaucet-app/server/internal/nitronode/client.gofaucet-app/server/internal/nitronode/client_validation_test.gofaucet-app/server/internal/server/ratelimiter.gofaucet-app/server/internal/server/server.gofaucet-app/server/internal/server/server_test.gofaucet-app/server/main.go
philanton
requested changes
May 21, 2026
Contributor
philanton
left a comment
philanton
left a comment
There was a problem hiding this comment.
Thanks for the migration! A bunch of stuff still needs to go before this can land — most of it is leftover from the standalone repo.
Please remove
faucet-app/.github/— only.github/at repo root is honored by Actions, these workflows never fire. Root CI already covers Go tests, tagging, releases.faucet-app/LICENSE— duplicate of root LICENSE.faucet-app/chart/— Helm chart with GKE config +ynet-stage/clearnet-*secret refs +canarynet.yellow.comhostnames. Belongs in infra repo, not OSS.faucet-app/scripts/— only consumed by the removed workflow, goes with it.
Critical bugs
faucet-app/server/.env.example— still referencesCLEARNODE_URLandSIGNER_PRIVATE_KEY.SIGNER_PRIVATE_KEYisn't read byconfig.goat all (single key drives both signers innitronode.NewClient). AndCOOLDOWN_PERIODis missing entirely, but it'senv-required:"true"— server fails to start from a fresh.env.example. Please rewrite.faucet-app/server/README.md— entire doc still says "Clearnode" (~30 hits), references the oldinternal/clearnodepackage name, wrong env var names, stale startup log examples. Please rewrite to matchinternal/nitronode.
Module layout
faucet-app/server/go.mod— please drop the nestedgo.mod/go.sumand fold faucet-app into the root module. Right now it's a separate module withreplace github.com/layer-3/nitrolite => ../../, which means rootgo test ./...doesn't cover faucet-app and CI never tests it. Also the module namefaucet-serverdoesn't follow the repo path convention — should begithub.com/layer-3/nitrolite/faucet-app/serverlikenitronode/,cerebro/,sdk/go/already are under the root module.
Code-level concerns
server/internal/nitronode/client.go:128-169—reconnectLockedholds the write lock acrosstime.Sleep(up to ~6s total) and the fullPinground-trip. All readers stall during a reconnect storm. I'd drop the lock before sleeping/pinging and re-acquire only to swapc.sdkClient.server/internal/nitronode/client.go:185-214—validateTokenSupportreadscachedunder RLock, then writestokenSupported=trueunder a separate Lock. Race window where N goroutines all callGetAssets()before one wins. Not catastrophic but worth tightening —sync.Onceper connection, or do the check+set under write lock.server/internal/server/ratelimiter.go:64-73—r.callsonly increments on accepted requests, so rejected (cooldown) requests don't trigger eviction. Map grows unbounded under sustained abuse from blocked IPs. I'd increment on rejection too.server/internal/server/server.go:69—SetTrustedProxies(nil)is fine for direct exposure but if the deployment puts the faucet behind any ingress/LB (which the chart implies),c.ClientIP()returns the proxy IP and the IP rate-limit collapses to one bucket. Please either expose aTRUSTED_PROXIESconfig or document the "direct exposure only" assumption.server/internal/nitronode/client.go:146-153—oldClient.Close()runs under the write lock. If SDK close does network I/O it stalls all readers. I'd swap the pointer under the lock, then defer the close until after unlock.
Tests pass locally otherwise. Once the cleanup + module flattening lands, the rest is small)
Contributor
Author
|
@philanton — addressed all your points in Removed
Module layout
|
ihsraham
reviewed
May 21, 2026
Collaborator
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
Looks close after the cleanup pass. I left a few focused inline notes. The Dockerfile is the main one I would fix before merge: go test ./faucet-app/... and go test -race ./faucet-app/... pass, but the Docker build currently fails after the module flattening.
| COPY . . | ||
|
|
||
| # Build the application | ||
| RUN CGO_ENABLED=0 GOOS=linux go build -a -installsuffix cgo -o faucet-server main.go |
Collaborator
There was a problem hiding this comment.
Flattening the faucet into the root module broke this Dockerfile. With repo root as context, this command runs in /app, where there is no main.go; with faucet-app/server as context, line 7 cannot find go.mod or go.sum.
I would update the image build to use repo-root context and build ./faucet-app/server explicitly.
Collaborator
There was a problem hiding this comment.
Still failing on the current head. I pulled 683804af and docker build -f faucet-app/server/Dockerfile . stops at this step because faucet-app/server/go.mod and go.sum were removed when the faucet was folded into the root module.
The Dockerfile should drop the nested module copy/download step and build from the root module, for example go build -o /build/bin/faucet-server ./faucet-app/server after copying the source tree.
|
|
||
| # Cooldown between requests per wallet/IP (Go duration format) | ||
| # Optional: default is 24h | ||
| COOLDOWN_PERIOD=your_cooldown_period_here (e.g., 24h, 1h, 30m) |
Collaborator
There was a problem hiding this comment.
These two lines still do not produce a usable fresh .env. Config requires COOLDOWN_PERIOD, but the comment says it is optional/defaulted, and this sample value is not a valid Go duration.
Please either add a real default in config or make the example something like COOLDOWN_PERIOD=24h and mark it required.
Collaborator
There was a problem hiding this comment.
This still looks unresolved on 683804af. The example now comments out COOLDOWN_PERIOD and still calls it optional, but config.Config marks it env-required:"true", so a fresh .env copied from this file will fail validation unless the user already has the variable in their shell.
Please either add an env-default:"24h" in config or make the example include an active value like COOLDOWN_PERIOD=24h and mark it required.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faucet-app/server/internal/nitronode/client.go (1)
315-320:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Close()is not actually serialized with reconnects.
Close()only holdsc.mu, while reconnect work is coordinated byreconnectMu. An in-flight reconnect can still swap in a fresh client around shutdown, leaving a live connection afterClose()returns.Proposed minimal fix
func (c *Client) Close() error { + c.reconnectMu.Lock() + defer c.reconnectMu.Unlock() + c.mu.Lock() - defer c.mu.Unlock() - return c.sdkClient.Close() + cl := c.sdkClient + c.mu.Unlock() + return cl.Close() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/internal/nitronode/client.go` around lines 315 - 320, Close() currently only acquires c.mu, which doesn't block the reconnect path protected by reconnectMu and can race in a new sdkClient; change Close() to acquire reconnectMu first and then c.mu (i.e., reconnectMu.Lock(); defer reconnectMu.Unlock(); then c.mu.Lock(); defer c.mu.Unlock()) so it serialises with the reconnect logic; ensure the established global lock ordering (reconnectMu before mu) is documented and that any reconnect code that touches c.sdkClient follows the same ordering to avoid deadlocks.
♻️ Duplicate comments (1)
faucet-app/server/internal/nitronode/client_validation_test.go (1)
33-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the error path contract explicitly.
When
err != nil, the test currently does not assert thatclientis nil, so a mixed(client != nil, err != nil)regression would still pass.Suggested assertion tightening
client, err := NewClient(key, "ws://localhost:19999", "usdc", decimal.NewFromInt(10), 1) if err == nil { require.NotNil(t, client) require.NoError(t, client.Close()) + } else { + require.Nil(t, client) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/internal/nitronode/client_validation_test.go` around lines 33 - 37, The test calling NewClient(...) doesn't assert the error-path contract for client when err != nil; update the test around NewClient, client, err so that if err != nil you explicitly require.Nil(t, client), and otherwise require.NotNil(t, client) and require.NoError(t, client.Close()); i.e., branch on err (or use require.NoError to assert success and then check client) to ensure no regression where client is non-nil while an error is returned.
🧹 Nitpick comments (1)
faucet-app/server/README.md (1)
117-117: 💤 Low valueAdd language identifier to fenced code block.
The fenced code block is missing a language identifier, flagged by markdownlint.
♻️ Proposed fix
-``` +```json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@faucet-app/server/README.md` at line 117, The fenced code block in README.md is missing a language identifier; update the opening triple-backtick for that fenced code block to include the language token (e.g., change ``` to ```json) so markdownlint recognizes it as JSON code and enables proper syntax highlighting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@faucet-app/server/Dockerfile`:
- Around line 8-10: Remove the now-invalid module-copy and download steps:
delete the COPY command that references "faucet-app/server/go.mod" and
"faucet-app/server/go.sum" and the subsequent "RUN cd faucet-app/server && go
mod download" from the Dockerfile, since the faucet-app was migrated to the root
module and dependencies are handled by the root go.mod already copied earlier.
In `@go.mod`:
- Line 62: Update the quic-go dependency from v0.54.0 to v0.57.0: edit the
go.mod entry for module github.com/quic-go/quic-go to use v0.57.0, then run `go
get github.com/quic-go/quic-go@v0.57.0` (or `go get -u`) and `go mod tidy` to
refresh transitive modules and lockfiles; finally rebuild/run tests to confirm
no breakages.
---
Outside diff comments:
In `@faucet-app/server/internal/nitronode/client.go`:
- Around line 315-320: Close() currently only acquires c.mu, which doesn't block
the reconnect path protected by reconnectMu and can race in a new sdkClient;
change Close() to acquire reconnectMu first and then c.mu (i.e.,
reconnectMu.Lock(); defer reconnectMu.Unlock(); then c.mu.Lock(); defer
c.mu.Unlock()) so it serialises with the reconnect logic; ensure the established
global lock ordering (reconnectMu before mu) is documented and that any
reconnect code that touches c.sdkClient follows the same ordering to avoid
deadlocks.
---
Duplicate comments:
In `@faucet-app/server/internal/nitronode/client_validation_test.go`:
- Around line 33-37: The test calling NewClient(...) doesn't assert the
error-path contract for client when err != nil; update the test around
NewClient, client, err so that if err != nil you explicitly require.Nil(t,
client), and otherwise require.NotNil(t, client) and require.NoError(t,
client.Close()); i.e., branch on err (or use require.NoError to assert success
and then check client) to ensure no regression where client is non-nil while an
error is returned.
---
Nitpick comments:
In `@faucet-app/server/README.md`:
- Line 117: The fenced code block in README.md is missing a language identifier;
update the opening triple-backtick for that fenced code block to include the
language token (e.g., change ``` to ```json) so markdownlint recognizes it as
JSON code and enables proper syntax highlighting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2807cb5f-ccc6-4e6a-a3da-17399312c7d4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
faucet-app/server/.env.examplefaucet-app/server/Dockerfilefaucet-app/server/README.mdfaucet-app/server/internal/config/config.gofaucet-app/server/internal/logger/logger.gofaucet-app/server/internal/nitronode/client.gofaucet-app/server/internal/nitronode/client_validation_test.gofaucet-app/server/internal/server/ratelimiter.gofaucet-app/server/internal/server/server.gofaucet-app/server/internal/server/server_test.gofaucet-app/server/main.gogo.mod
✅ Files skipped from review due to trivial changes (1)
- faucet-app/server/.env.example
Comment on lines
+8
to
+10
| # Copy faucet-app/server module files and pre-download dependencies | ||
| COPY faucet-app/server/go.mod faucet-app/server/go.sum ./faucet-app/server/ | ||
| RUN cd faucet-app/server && go mod download |
Contributor
There was a problem hiding this comment.
Remove references to deleted module files.
Lines 8-10 attempt to copy faucet-app/server/go.mod and go.sum, but per the PR objectives these files were removed when the faucet-app was migrated to use the root module. The Docker build will fail with a "file not found" error.
🐛 Proposed fix
Since the faucet-app now uses the root module, remove these lines entirely. Dependencies are already downloaded via the root go.mod copied at line 6:
-# Copy faucet-app/server module files and pre-download dependencies
-COPY faucet-app/server/go.mod faucet-app/server/go.sum ./faucet-app/server/
-RUN cd faucet-app/server && go mod download
+# Pre-download dependencies from root module
+RUN go mod download🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@faucet-app/server/Dockerfile` around lines 8 - 10, Remove the now-invalid
module-copy and download steps: delete the COPY command that references
"faucet-app/server/go.mod" and "faucet-app/server/go.sum" and the subsequent
"RUN cd faucet-app/server && go mod download" from the Dockerfile, since the
faucet-app was migrated to the root module and dependencies are handled by the
root go.mod already copied earlier.
| github.com/pelletier/go-toml/v2 v2.2.4 // indirect | ||
| github.com/pkg/term v1.2.0-beta.2 // indirect | ||
| github.com/quic-go/qpack v0.5.1 // indirect | ||
| github.com/quic-go/quic-go v0.54.0 // indirect |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the first patched version of github.com/quic-go/quic-go that fixes GO-2025-4017 and GO-2025-4233?
💡 Result:
First patched versions of github.com/quic-go/quic-go that fix GO-2025-4017 and GO-2025-4233: GO-2025-4017: v0.54.1 [1]. GO-2025-4233: v0.57.0 [2][3].
Citations:
- 1: https://pkg.go.dev/vuln/GO-2025-4017
- 2: https://osv.dev/vulnerability/GHSA-g754-hx8w-x2g6
- 3: https://nvd.nist.gov/vuln/detail/CVE-2025-64702
Upgrade github.com/quic-go/quic-go from v0.54.0.
v0.54.0 is affected by GO-2025-4017 and GO-2025-4233; the patched versions are v0.54.1 (GO-2025-4017) and v0.57.0 (GO-2025-4233). Bump github.com/quic-go/quic-go to v0.57.0 to address both advisories via transitive HTTP/3 paths.
🧰 Tools
🪛 OSV Scanner (2.3.8)
[HIGH] 62-62: github.com/quic-go/quic-go 0.54.0: Panic occurs when queuing undecryptable packets after handshake completion in github.com/quic-go/quic-go
(GO-2025-4017)
[HIGH] 62-62: github.com/quic-go/quic-go 0.54.0: HTTP/3 QPACK Header Expansion DoS in github.com/quic-go/quic-go
(GO-2025-4233)
[HIGH] 62-62: github.com/quic-go/quic-go 0.54.0: quic-go: Panic occurs when queuing undecryptable packets after handshake completion
[HIGH] 62-62: github.com/quic-go/quic-go 0.54.0: quic-go HTTP/3 QPACK Header Expansion DoS
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` at line 62, Update the quic-go dependency from v0.54.0 to v0.57.0:
edit the go.mod entry for module github.com/quic-go/quic-go to use v0.57.0, then
run `go get github.com/quic-go/quic-go@v0.57.0` (or `go get -u`) and `go mod
tidy` to refresh transitive modules and lockfiles; finally rebuild/run tests to
confirm no breakages.
Collaborator
There was a problem hiding this comment.
Verified this one locally. go run golang.org/x/vuln/cmd/govulncheck@latest ./faucet-app/... reports GO-2025-4233 as reachable through faucet-app/server -> gin -> quic-go/http3, and the Go vulnerability database lists github.com/quic-go/quic-go@v0.57.0 as the fixed version.
Since this PR introduces the Gin dependency that pulls in quic-go, I would bump github.com/quic-go/quic-go before merge rather than leave the vulnerable transitive version in the root module.
Contributor
There was a problem hiding this comment.
@ihsraham, thanks for the local verification — that confirms the vulnerability is reachable, not just a transitive phantom. Here's the minimal fix:
go.mod change:
- github.com/quic-go/qpack v0.5.1 // indirect
- github.com/quic-go/quic-go v0.54.0 // indirect
+ github.com/quic-go/qpack v0.6.0 // indirect
+ github.com/quic-go/quic-go v0.57.0 // indirectNote: quic-go v0.56.0 updated qpack to v0.6.0, so qpack needs to be bumped alongside quic-go to keep the dependency graph consistent.
After editing go.mod, regenerate the lockfile from the repo root:
go get github.com/quic-go/quic-go@v0.57.0
go mod tidy
go test ./faucet-app/... # confirm no breakage
govulncheck ./faucet-app/... # confirm advisories are clearedSince quic-go is an indirect dependency here (Gin pulls it in for HTTP/3 transport, not the faucet server directly), the bump doesn't touch any faucet-app source files — only go.mod and go.sum change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's included
5aeb7281— General migrationCopies the faucet-app server into
faucet-app/inside this repo. Switches the Go module from the publishedgithub.com/erc7824/nitrolite v1.1.1to local code via areplacedirective pointing at../../, so changes topkg/andsdk/goare immediately picked up without a publish step.b5f82223— Rename: clearnode → nitronodePure textual rename across all files: package name, directory (
internal/clearnode→internal/nitronode), identifiers (ClearnodeClient→NitronodeClient,clearnodeURL→nitronodeURL, env varCLEARNODE_URL→NITRONODE_URL), log messages, config keys in Helm values, and README references. No logic changes.06dcb25b— Client hardening fixesImprovements to
internal/nitronode/client.goidentified during a review against the currentsdk/goAPI:reconnectLockedretries up to 3 times (300 ms → 600 ms → 2 s) instead of giving up immediately; each attempt includes a post-connectPingto confirm liveness before accepting the new client.WithApplicationID("faucet")— tags all requests to the node for server-side observability.WithErrorHandler— connection errors are now routed through the faucet logger instead of being silently dropped.validateTokenSupportcallsGetAssetsonly once per connection; result is stored intokenSupportedand reset on reconnect, eliminating an unnecessary RPC round-trip on every HTTP request.Close()uses write lock — prevents a race whereClose()could close a freshly reconnected client instead of the stale one.ownerAddresscached at construction —GetOwnerAddress()no longer takes a lock; the address is derived from the immutable signer and never changes across reconnects.Enforcedbalance warning — logs a warning when the on-chain enforced balance is below the channel balance, signalling uncheckpointed funds at risk."0"fallback removed —Transferno longer silently replaces a server-reported zero amount with the configured tip amount.Summary by CodeRabbit
Release Notes
New Features
/requestTokensand/infoREST API endpointsDocumentation
Chores