Geolocation: Complete Go Write SDK

[ben-dz]

Combine the requests in issue #3296 and #3011, into a Geolocation Go Write SDK to match the Rust equivalents. Issue #3011 was worked on and has a draft branch that can be used or not based on what makes sense given the evolution of the code since then. The Read half of the SDK already exists and complete enough.

The Go Write SDK does not need to include everything. It can skip Foundation commands, such as probe creation and management, marking users at paid etc. That will continue to be CLI, or possible rust automations in the future. There's no need for Go code to do that. This should just be the SDK a user would need to automate adding/removing targets from their (already existing) user account, and possible changing their offset destination, or other behaviors that a user might want to automate.

[ben-dz]

Architecture review findings:

• Critical: 0
• High: 2 (both resolved)
• Medium: 3
• Low: 2

Architecture Review

Critical

None

High

• [Borsh Serialization - Validation Gap]: AddTargetInstructionConfig.Validate() does not validate that TargetType is a known value (0, 1, or 2). A caller could pass, e.g., GeoLocationTargetType(255), which would bypass the switch validation on IP address (since it only checks Outbound/OutboundIcmp and Inbound) and produce an instruction the Rust program will reject at deserialization time. While the onchain program would reject it, failing early with a clear error message is better for SDK users.

  • Location: sdk/geolocation/go/add_target.go:19-42 and sdk/geolocation/go/remove_target.go:19-33
  • Recommendation: Add a default case in the AddTarget switch or a standalone target-type validation. In RemoveTarget.Validate(), add target-type validation as well.
  • Resolution: Fixed in commit 724a6289 — added default case in AddTarget switch and TargetType > GeoLocationTargetTypeOutboundIcmp check in RemoveTarget.

• [Context Cancellation]: waitForSignatureVisible uses time.Sleep(250ms) in a loop without checking ctx.Done(). If the caller cancels the context, the function will continue polling until the deadline expires rather than returning promptly. The sibling method waitForTransactionFinalized correctly uses select on ctx.Done().

  • Location: sdk/geolocation/go/executor.go:119-133
  • Recommendation: Replace time.Sleep(250 * time.Millisecond) with a select { case <-ctx.Done(): return ctx.Err() case <-time.After(250 * time.Millisecond): } block.
  • Resolution: Fixed in commit 724a6289 — replaced time.Sleep with select on ctx.Done().

Medium

• [API Design - Unexported Return Type]: NewExecutor returns *executor (unexported type). While this matches the telemetry SDK pattern, it means callers outside the package cannot declare variables of this type. This is consistent with the existing telemetry SDK, so changing it here alone would create inconsistency.

  • Location: sdk/geolocation/go/executor.go:35
  • Recommendation: Consider introducing an exported Executor interface in a follow-up PR that updates both the geolocation and telemetry SDKs.

• [Validation Asymmetry]: AddTargetInstructionConfig.Validate() performs client-side IP validation for outbound target types, but SetResultDestinationInstructionConfig does not validate Destination format, and CreateGeolocationUserInstructionConfig does not validate that TokenAccount is non-zero. The current approach is defensible (validate what the SDK can validate cheaply, let the program handle domain-specific validation).

  • Location: Various instruction builder files
  • Recommendation: Document the expected format for Destination in the config struct's godoc.

• [Test Coverage - No Mock Executor Tests]: The executor tests only cover constructor creation and early guard conditions. There are no tests for the happy path through ExecuteTransactions with a mock ExecutorRPCClient.

  • Location: sdk/geolocation/go/executor_test.go
  • Recommendation: Add a test with a mock ExecutorRPCClient in a follow-up.

Low

• [Naming - Config Prefix Inconsistency]: Some config types use the full instruction name as prefix (CreateGeolocationUserInstructionConfig) while others use a shorter form (AddTargetInstructionConfig). Both forms are readable.

  • Location: Various config type definitions
  • Recommendation: No action required.

• [Documentation]: The instruction builder functions lack godoc comments.

  • Location: All Build* functions
  • Recommendation: Add one-line godoc comments to each exported Build* function and config struct.

Summary

The write SDK is well-structured and follows established patterns from the telemetry SDK consistently. Borsh serialization layouts correctly match the Rust program's instruction enum definitions. Both high-severity findings have been resolved in a follow-up commit. The change does not break any existing read-SDK functionality.

[ben-dz]

Security review findings:

• Critical: 0
• High: 0
• Medium: 3
• Low: 4

Security Review

Critical

None

High

None

Medium

• [Input Validation - Missing TargetType enum validation]: The AddTargetInstructionConfig.Validate() and RemoveTargetInstructionConfig.Validate() methods did not reject invalid TargetType values. An arbitrary uint8 value could be serialized into the instruction data and submitted onchain.

  • Location: sdk/geolocation/go/add_target.go:30-39 and sdk/geolocation/go/remove_target.go:19-33
  • Recommendation: Add validation for unknown target types.
  • Resolution: Fixed in commit 724a6289.

• [Input Validation - Incomplete private IP range blocking]: The validateNotPrivateIP function blocks RFC 1918 ranges, loopback, and 0.0.0.0, but omits link-local (169.254.0.0/16), multicast (224.0.0.0/4), reserved (240.0.0.0/4), and broadcast (255.255.255.255). The onchain Rust program has its own validate_public_ip function that provides additional checks.

  • Location: sdk/geolocation/go/add_target.go:45-67
  • Recommendation: The SDK provides defense-in-depth validation; the onchain program is authoritative. Additional ranges could be added in a follow-up.

• [Denial of Service - Unbounded finalization polling loop]: waitForTransactionFinalized polls indefinitely bounded only by the caller-provided context. If a caller passes context.Background(), the function will spin forever.

  • Location: sdk/geolocation/go/executor.go:135-159
  • Recommendation: Document that callers must provide a context with a deadline, or add a configurable max timeout.

Low

• [Input Validation - Unbounded Destination string]: SetResultDestinationInstructionConfig.Validate() does not enforce maximum length on Destination. Solana's transaction size limit provides a natural upper bound.

  • Location: sdk/geolocation/go/set_result_destination.go:16-24
  • Recommendation: Consider adding a reasonable upper bound.

• [Input Validation - Unbounded ProbePKs slice]: SetResultDestinationInstructionConfig.ProbePKs has no upper bound check. A very large slice could exceed Solana's account limit.

  • Location: sdk/geolocation/go/set_result_destination.go:54-58
  • Recommendation: Add a reasonable upper bound on the number of ProbePKs.

• [Input Validation - No UTF-8 validation on Code string]: The Code string is validated for emptiness and byte-length but not for valid UTF-8 encoding.

  • Location: All Validate() methods checking Code
  • Recommendation: Consider adding utf8.ValidString(c.Code) check.

• [Potential Resource Leak - Timer in polling loop]: In waitForTransactionFinalized, time.After creates a new timer channel each iteration that is not explicitly stopped. This is a minor resource concern.

  • Location: sdk/geolocation/go/executor.go:154
  • Recommendation: Consider using time.NewTimer with explicit Stop().

Summary

The SDK is well-structured with consistent input validation patterns. No secrets exposure, injection, or authentication vulnerabilities were identified. Private key handling is appropriately scoped to the executor. The primary concerns are enum bounds validation (now fixed) and documentation of context deadline requirements for callers.