refactor(go): idiomatic Go cleanup across the SDK#214
Conversation
Greptile SummaryThis PR performs a comprehensive idiomatic Go cleanup pass over the SDK: fixing four real bugs (error propagation in the MPP compute-budget path, session-store lock-map leak, dropped pubkey-decode errors in
Confidence Score: 4/5Safe to merge with one known residual gap: the usage-adapter 402 path still adds its accepts entry and challenge headers in two independent calls, so a partial offer can still be emitted if one succeeds and the other fails — the same asymmetry the regular adapter path now avoids via the atomic appendChallenge helper. All four stated bug fixes are correct and well-tested: compute-budget errors now propagate instead of being swallowed, the session-store lock map is cleaned up on deletion, signerBridge decodes the pubkey once where errors can surface, and the adapter interface changes propagate build failures to callers. The appendChallenge refactor makes the regular 402 path properly atomic. The functional-options challenge constructor eliminates a class of silent positional-argument bugs. The go fix CI gate enforces ongoing modernization. The one remaining gap is in writeUsage402 and the settlement-error branch of RequireUsageFunc, where UsageAcceptsEntry and UsageChallengeHeaders are still called and committed independently — a pattern the regular path specifically avoids. go/paykit/usage.go — writeUsage402 and the settlement-error branch in RequireUsageFunc should apply the same all-or-nothing logic used by appendChallenge in the regular adapter path. Important Files Changed
Reviews (10): Last reviewed commit: "ci(ruby): refresh audit-safe dependencie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| engine.rpc = fakeRPC | ||
|
|
There was a problem hiding this comment.
RPCClient config field: These tests still assign engine.rpc directly (an unexported field) rather than using the new UptoConfig.RPCClient field that this PR introduces to replace SetRPCForTests. This pattern appears across all upto_test.go test cases that need a fake RPC. The stated goal of adding RPCClient was to give callers (including tests) a clean injection point, but the tests themselves don't exercise that path — so the new field is untested through its public API.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
MemoryChannelStore.DeleteChannel deleted from s.data but never s.locks, so the per-channel mutex map grew without bound for the process lifetime; delete from both under the same mu. BuildCredential silently skipped the compute-unit price/limit instructions when their builders errored (if ix, err := ...; err == nil), so a charge could be signed without a compute-unit limit. Propagate the error like the surrounding code does.
…ckages strconv.FormatUint/ParseUint over fmt.Sprintf/Sscanf for integer conversion, drop single-case switch and redundant []byte and import aliases, and MixedCaps/initialism fixes. No behavior change.
Collapse NewChallengeWithSecret/NewChallengeWithSecretFull into one constructor whose optional expires/digest/description/opaque fields are set via named ChallengeOption values, so the three same-typed optional strings can no longer be transposed silently at a call site. Also fix the wire package doc comment (was 'core') and rename the cap parameter that shadowed the builtin.
PaymentIdentifierInfo.Id becomes ID; the JSON tag stays 'id' so the wire shape is unchanged.
Drop the test-only SetRPCForTests method from the public X402Upto and inject the RPC client through a new UptoConfig.RPCClient field instead. Extract the adapter's paykit.Config to UptoConfig mapping into buildUptoConfig so the adapter test can inject a deterministic client without a public test seam.
Adapter.AcceptsEntry/ChallengeHeaders and UsageAdapter now return (value, error) so a base58 or serialization failure surfaces to the caller instead of being swallowed. The 402 challenge writer logs and drops the offending protocol rather than emitting a half-built header, and signerBridge decodes the operator pubkey once at construction where the error can propagate. Move ContextWith*ForTests helpers out of the production surface into export_test.go.
The interface refactor added error-return branches (adapter accepts/ challenge builders, RPC config, binding checks) that dropped statement coverage below the 91% CI gate. Add in-package tests for the exact and mpp adapters (Protocol, ChallengeHeaders, VerifyAcceptedBinding, normalizeNetwork, missing-secret and bad-recipient constructors) and the 402 challenge-writer error-drop paths so each new branch is exercised.
The upto tests set engine.rpc directly (an unexported field) after construction, which bypassed the public RPCClient config field this PR introduced to replace SetRPCForTests. Pass RPCClient in the UptoConfig literal so the tests exercise the same injection path real callers use.
Ran go fix across the Go SDK and harness modules: interface{} -> any, the
slices/maps stdlib helpers, strings.SplitSeq iteration, min/max builtins, and
range-over-int/loopvar rewrites. Mechanical, no behavior change; the generated
codama client under protocols/programs/paymentchannels is left untouched.
build, vet, gofmt, and the full go test suite pass.
Add scripts/check-go-fix.sh and run it in the Go lint job so CI fails when the Go 1.26 language/stdlib modernizers (go fix -inline=false) would still change any non-generated source, keeping the SDK on newer idioms (any, min/max, new(expr), range-over-int, slices/maps, SplitSeq, wg.Go) without manual review comments. The inline analyzer is excluded because it applies opt-in //go:fix inline directives and is cache sensitive; generated codama clients are excluded by their AUTOGENERATED header. On failure the gate prints the suggested diff so the fix is copy-pasteable.
EfeDurmaz16
force-pushed
the
fix/go-idiomatic-cleanup
branch
from
94ee9db to
6ff0a6c
Compare
|
@greptile-apps review |
|
@greptile-apps please run a fresh full review of the latest commit. |
|
Merge conflicts |
2 participants
What
A pass over the Go SDK to bring it in line with idiomatic Go, sourced from the
go.dev Code Review Comments, Effective Go, and the Uber Go Style Guide. The
work started from a line-by-line read of every non-generated
.gofile andgrouped the findings into four real bugs, a set of structural fixes, a style
sweep, and a Go 1.26 modernization pass that is now enforced in CI. Codama-
generated code under
protocols/programs/paymentchannelsis left untouched.Wire shapes are unchanged: the
IdtoIDrename keeps itsjson:"id"tag,and the challenge-builder refactor produces byte-identical output. The
cross-SDK interop harness and the existing unit suites cover the behavior.
Commits
Each commit is one logical step and stands on its own:
and limit builders, and the session-store lock map, were discarding errors
and leaking map entries.
Adapter.AcceptsEntry/ChallengeHeadersandUsageAdapternow return(value, error), so a base58 or serialization failure reaches the caller.The 402 writer logs and drops the offending protocol rather than emitting a
half-built header,
signerBridgedecodes the operator pubkey once atconstruction where the error can propagate, and the test-only
ContextWith*ForTestshelpers move toexport_test.go.test-only method from the public
X402Uptosurface and injects the RPCclient through a new
UptoConfig.RPCClientfield instead.PaymentIdentifierInfo.Idbecomes
ID; the JSON tag staysid.NewChallengeWithSecret/NewChallengeWithSecretFullinto one constructorwhose optional
expires/digest/description/opaquefields are set vianamed
ChallengeOptionvalues, so the three same-typed optional strings canno longer be transposed at a call site. Also fixes the
wirepackage doccomment (was
core) and renames a parameter that shadowed thecapbuiltin.strconv.FormatUint/ParseUintoverfmt.Sprintf/Sscanf, dropping asingle-case switch, a redundant
[]byteconversion, and a redundant importalias.
across every non-generated file:
interface{}toany,if/elsetomin/max, 3-clause counters tofor i := range n,slices/mapshelpers,and
strings.SplitSeq. Includes the one loop the rebase pulled in frommain's concurrent-replay test (fix(mpp): close concurrent-signature-replay TOCTOU in TS charge verify #211), so the gate below starts green.go/scripts/check-go-fix.sh, which fails whengo fix -inline=false ./...would still change any non-generated source, so the modernization can't
silently regress. The
inlineanalyzer is excluded (it applies opt-in//go:fix inlinedirectives and is cache sensitive); generated codama clientsare excluded by their header. On failure the gate prints the suggested diff.
Verifying
Self-review notes
RPCClientfield is a legitimate public option (custom transport), not adisguised test hook. The single cross-package caller that needed the old
method now builds its engine through the extracted
buildUptoConfighelper.test suites; those are updated in the same commit so each commit builds.
ProcessOpen/ProcessClosecarry new doc comments flagging that the HTTPmethod layer deliberately reimplements those invariants as a superset; this
is documentation only, no code path changed.
main, so its CI runs against the real mergebase. The gate is scoped to the deterministic modernizers on purpose: an
earlier version that ran the full
go fixflagged an un-modernized loop thatlived on
mainrather than in this PR, which would fail unrelated Go PRs onbase-branch drift.