feat: add RPC API version discovery with multi-version negotiation#288
Open
feat: add RPC API version discovery with multi-version negotiation#288
Conversation
Contributor
WalkthroughAdds multi-version discovery: new protobuf types (ModuleVersion, VersionInfo), extends Akash proto with supported_versions and metadata, adds Discovery/GetInfo RPC, implements a concurrency-safe VersionRegistry and registry-backed discovery server/client, adds v1beta4 client aliases, negotiation logic preferring v1beta4→v1beta3, tests, mocks, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Discovery as "Discovery gRPC/HTTP"
participant Registry as "VersionRegistry"
participant Factory as "ClientFactory"
Client->>Discovery: GetInfo (HTTP/gRPC / JSON-RPC)
Discovery->>Registry: ToAkash()
Registry-->>Discovery: Akash proto (supported_versions, client_info, metadata)
Discovery-->>Client: GetInfoResponse.info
Note over Client,Factory: Client-side negotiation & client creation
Client->>Client: negotiateVersion(Akash)
alt supported_versions present
Client->>Client: pick newest compatible (prefer v1beta4 → v1beta3)
else supported_versions empty
Client->>Client: fallback to client_info.api_version
end
Client->>Factory: New client for negotiated version
Factory-->>Client: versioned client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 6
🧹 Nitpick comments (2)
go/node/client/registry.go (1)
94-104: Make version-ordering contract explicit forOldestVersion().
OldestVersion()depends on slice order, butNewRegistry()accepts arbitrary input. Consider documenting/enforcing the expected order (newest→oldest) to avoid accidental legacy-version regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/registry.go` around lines 94 - 104, OldestVersion() assumes r.supportedVersions is ordered newest→oldest but NewRegistry() currently accepts arbitrary input; make the contract explicit and enforce it by sorting or validating on construction: update NewRegistry() (or the VersionRegistry constructor) to sort r.supportedVersions by semantic version (or document and validate order) so the last element is always the oldest, and add a comment on VersionRegistry/OldestVersion() stating the required newest→oldest ordering; reference VersionRegistry.supportedVersions, NewRegistry(), and OldestVersion() when implementing the sort/validation and adding the comment.go/node/client/client.go (1)
39-42: Keep negotiation order and factories in one source of truth.Adding
v1beta4already required updatingclientPreferenceOrderplus three switch statements. A single ordered version descriptor would let negotiation and instantiation share the same data and remove the easy-to-miss drift whenv1beta5+arrives.Also applies to: 101-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/client.go` around lines 39 - 42, clientPreferenceOrder is duplicated and several switch-based factories (referenced around the code handling VersionV1beta4/VersionV1beta3 and the switch blocks at the other instantiation sites) drift when new versions are added; replace the separate clientPreferenceOrder slice with a single ordered registry of version descriptors (each descriptor containing the version constant like VersionV1beta4/VersionV1beta3 and a factory function) and use that registry both for negotiation and for creating clients/servers instead of the three switch statements, removing the duplicate version lists and centralizing instantiation logic so adding v1beta5+ requires a single update to the registry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proto/node.md`:
- Line 3693: Generated discovery section headings (e.g., the "ModuleVersion"
heading in the Discovery block) are emitted with leading indentation causing
MD023 warnings; update the generator template that produces docs/proto/node.md
so that Markdown headings are emitted with no leading spaces (headings start at
column 1) for all Discovery headings, or alternatively configure the linter to
ignore MD023 for this generated file; ensure the change affects all occurrences
of those discovery headings so the generator emits top-level headings
consistently.
In `@go/node/client/client.go`:
- Around line 195-205: defaultRegistry is accessed concurrently unsafely by
SetRegistry, GetRegistry and RPCAkash and SetRegistry allows nil; introduce a
package-level sync.RWMutex (e.g., registryMu) to guard reads/writes of
defaultRegistry, update SetRegistry to acquire registryMu.Lock(), reject nil
replacements (return or panic per project style), assign defaultRegistry and
unlock; update GetRegistry to use registryMu.RLock()/RUnlock() and return the
pointer safely; update RPCAkash to RLock(), check defaultRegistry for nil and
return an error if nil, otherwise call defaultRegistry.ToAkash() while still
holding the read lock (or copy the pointer into a local variable under lock and
call ToAkash() after unlocking) to avoid a race or nil-deref.
In `@go/node/client/discovery_server.go`:
- Around line 17-24: NewDiscoveryServer currently accepts a nil *VersionRegistry
which causes a panic later in GetInfo when calling s.registry.ToAkash(); add a
fast-fail or default at construction by checking if registry == nil inside
NewDiscoveryServer: either return a sensible default VersionRegistry (e.g., call
NewVersionRegistry() if available) or immediately panic/log.Fatalf with a clear
message like "NewDiscoveryServer: nil registry provided" so the server cannot be
created with a nil registry; update discoveryServer creation to always hold a
non-nil registry so GetInfo (and s.registry.ToAkash()) cannot panic at request
time.
In `@go/node/client/README.md`:
- Around line 180-184: The fenced code block that shows "Client preference:
[v1beta4, v1beta3] (newest first)" etc. lacks a language tag; update that
markdown fence by adding a language identifier (e.g., change the opening "```"
to "```text") for the block in README.md so markdownlint stops flagging it and
the docs render properly.
- Around line 161-164: The example hard-casts the discovered client to a
concrete version type (v1beta3.Client) which will break when versions diverge;
change the example to assert into a shared interface or an untyped interface{}
and then convert via a version-agnostic interface. Specifically, replace the
direct cast in the aclient.DiscoverClient callback (where myClient =
cl.(v1beta3.Client)) with an assignment to a shared interface variable (e.g.,
var myClient SomeCommonClientInterface or var myClient interface{}) and document
that callers should assert to the concrete version only after checking
capabilities; keep DiscoverClient and the opts usage but do not depend on
v1beta3.Client in the example.
In `@go/node/client/registry.go`:
- Around line 41-44: NewRegistry is taking the caller's versions slice as-is,
which lets the caller mutate the registry's backing array; make a defensive copy
of the incoming []VersionInfo into a new slice and store that in
VersionRegistry.supportedVersions (also apply the same clone when any
RegistryOption mutates or reads the slice). Likewise, any method that returns
the internal slice (e.g., ToAkash and any SupportedVersions accessor) must
return a copy of the slice (and if VersionInfo is mutable, copy elements as
well) so the registry's internal backing array cannot be aliased by callers;
update NewRegistry, ToAkash, and the related functions at the referenced
locations to perform these copies.
---
Nitpick comments:
In `@go/node/client/client.go`:
- Around line 39-42: clientPreferenceOrder is duplicated and several
switch-based factories (referenced around the code handling
VersionV1beta4/VersionV1beta3 and the switch blocks at the other instantiation
sites) drift when new versions are added; replace the separate
clientPreferenceOrder slice with a single ordered registry of version
descriptors (each descriptor containing the version constant like
VersionV1beta4/VersionV1beta3 and a factory function) and use that registry both
for negotiation and for creating clients/servers instead of the three switch
statements, removing the duplicate version lists and centralizing instantiation
logic so adding v1beta5+ requires a single update to the registry.
In `@go/node/client/registry.go`:
- Around line 94-104: OldestVersion() assumes r.supportedVersions is ordered
newest→oldest but NewRegistry() currently accepts arbitrary input; make the
contract explicit and enforce it by sorting or validating on construction:
update NewRegistry() (or the VersionRegistry constructor) to sort
r.supportedVersions by semantic version (or document and validate order) so the
last element is always the oldest, and add a comment on
VersionRegistry/OldestVersion() stating the required newest→oldest ordering;
reference VersionRegistry.supportedVersions, NewRegistry(), and OldestVersion()
when implementing the sort/validation and adding the comment.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5901d1f0-164e-4c30-b656-68bfa00f6d7d
⛔ Files ignored due to path filters (13)
go/node/client/akash.pb.gois excluded by!**/*.pb.gogo/node/client/client_info.pb.gois excluded by!**/*.pb.gogo/node/client/service.pb.gois excluded by!**/*.pb.gogo/node/client/service.pb.gw.gois excluded by!**/*.pb.gw.gors/src/gen/node/akash.discovery.v1.rsis excluded by!**/gen/**rs/src/gen/node/akash.discovery.v1.serde.rsis excluded by!**/gen/**rs/src/gen/node/akash.discovery.v1.tonic.rsis excluded by!**/gen/**ts/src/generated/createNodeSDK.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/akash.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/client_info.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/service.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/service_akash.tsis excluded by!**/generated/**ts/src/generated/protos/index.akash.v1.tsis excluded by!**/generated/**
📒 Files selected for processing (13)
docs/proto/node.mdgo/node/client/README.mdgo/node/client/client.gogo/node/client/discovery/client.gogo/node/client/discovery_server.gogo/node/client/negotiation_test.gogo/node/client/registry.gogo/node/client/registry_test.gogo/node/client/v1beta4/client.gogo/testutil/cmocks.goproto/node/akash/discovery/v1/akash.protoproto/node/akash/discovery/v1/client_info.protoproto/node/akash/discovery/v1/service.proto
troian
force-pushed
the
client-discovery
branch
from
6493134 to
9de71a6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/proto/node.md (1)
3691-3813:⚠️ Potential issue | 🟡 MinorFix the generator's indented discovery headings.
These new discovery headings still start with leading whitespace, so markdownlint MD023 will keep failing. Since this file is generated, please fix the template or exclude this file/rule instead of editing the output by hand.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proto/node.md` around lines 3691 - 3813, Generated markdown headings in docs/proto/node.md (e.g., "akash.discovery.v1.ModuleVersion", "akash.discovery.v1.Akash", "akash.discovery.v1.Discovery") include leading whitespace causing markdownlint MD023 failures; update the generator/template to trim leading spaces when emitting section anchors and "###" headings (or alter the generator to emit left-aligned headings), and if changing the generator is not feasible add this file to the lint exclude list for MD023 in the linter config so generated output is not flagged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go/node/client/client.go`:
- Around line 45-64: The code is rejecting responses that include
SupportedVersions because queryClientInfo unconditionally errors if
ClientInfo.ApiVersion is empty; update queryClientInfo so it only requires
ClientInfo.ApiVersion when SupportedVersions is empty (i.e., if
len(result.SupportedVersions) == 0 and result.ClientInfo.ApiVersion == "" return
ErrDetectClientVersion), otherwise allow negotiateVersion(result) to run and
pick from result.SupportedVersions using clientPreferenceOrder; keep
ErrDetectClientVersion for the legacy path or when negotiateVersion returns an
empty string.
In `@go/testutil/cmocks.go`:
- Around line 50-51: MockCometRPC.Akash currently returns a fresh
arpcclient.DefaultRegistry(), bypassing any registry set via SetRegistry; update
MockCometRPC.Akash to return GetRegistry().ToAkash() instead so tests observe
custom chain_id/node_version/min_client_version and supported-version sets the
same way RPCAkash (in go/node/client/client.go) does. Locate the
MockCometRPC.Akash method and replace the DefaultRegistry() call with
GetRegistry() to mirror production discovery.
---
Duplicate comments:
In `@docs/proto/node.md`:
- Around line 3691-3813: Generated markdown headings in docs/proto/node.md
(e.g., "akash.discovery.v1.ModuleVersion", "akash.discovery.v1.Akash",
"akash.discovery.v1.Discovery") include leading whitespace causing markdownlint
MD023 failures; update the generator/template to trim leading spaces when
emitting section anchors and "###" headings (or alter the generator to emit
left-aligned headings), and if changing the generator is not feasible add this
file to the lint exclude list for MD023 in the linter config so generated output
is not flagged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b513553f-faca-4bca-885d-3679322edb30
⛔ Files ignored due to path filters (13)
go/node/client/akash.pb.gois excluded by!**/*.pb.gogo/node/client/client_info.pb.gois excluded by!**/*.pb.gogo/node/client/service.pb.gois excluded by!**/*.pb.gogo/node/client/service.pb.gw.gois excluded by!**/*.pb.gw.gors/src/gen/node/akash.discovery.v1.rsis excluded by!**/gen/**rs/src/gen/node/akash.discovery.v1.serde.rsis excluded by!**/gen/**rs/src/gen/node/akash.discovery.v1.tonic.rsis excluded by!**/gen/**ts/src/generated/createNodeSDK.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/akash.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/client_info.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/service.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/service_akash.tsis excluded by!**/generated/**ts/src/generated/protos/index.akash.v1.tsis excluded by!**/generated/**
📒 Files selected for processing (13)
docs/proto/node.mdgo/node/client/README.mdgo/node/client/client.gogo/node/client/discovery/client.gogo/node/client/discovery_server.gogo/node/client/negotiation_test.gogo/node/client/registry.gogo/node/client/registry_test.gogo/node/client/v1beta4/client.gogo/testutil/cmocks.goproto/node/akash/discovery/v1/akash.protoproto/node/akash/discovery/v1/client_info.protoproto/node/akash/discovery/v1/service.proto
✅ Files skipped from review due to trivial changes (3)
- go/node/client/negotiation_test.go
- go/node/client/registry_test.go
- go/node/client/v1beta4/client.go
🚧 Files skipped from review as they are similar to previous changes (5)
- go/node/client/discovery/client.go
- go/node/client/discovery_server.go
- proto/node/akash/discovery/v1/akash.proto
- proto/node/akash/discovery/v1/client_info.proto
- go/node/client/registry.go
troian
force-pushed
the
client-discovery
branch
from
9de71a6 to
713830d
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
go/node/client/registry.go (1)
41-44:⚠️ Potential issue | 🔴 CriticalClone
supportedVersionson both ingress and egress.
NewRegistrystores caller-owned slices by reference, andToAkashreturns the same backing arrays to callers. That lets external code mutateSupportedVersions/Modulesoutside the mutex and corrupt shared registry state.🔒 Proposed fix
+func cloneModuleVersions(in []ModuleVersion) []ModuleVersion { + if len(in) == 0 { + return nil + } + out := make([]ModuleVersion, len(in)) + copy(out, in) + return out +} + +func cloneVersionInfos(in []VersionInfo) []VersionInfo { + if len(in) == 0 { + return nil + } + out := make([]VersionInfo, len(in)) + for i := range in { + out[i].ApiVersion = in[i].ApiVersion + out[i].Modules = cloneModuleVersions(in[i].Modules) + out[i].Features = append([]string(nil), in[i].Features...) + } + return out +} + func NewRegistry(versions []VersionInfo, opts ...RegistryOption) *VersionRegistry { r := &VersionRegistry{ - supportedVersions: versions, + supportedVersions: cloneVersionInfos(versions), } for _, opt := range opts { opt(r) @@ return &Akash{ ClientInfo: ClientInfo{ApiVersion: oldest}, - SupportedVersions: r.supportedVersions, + SupportedVersions: cloneVersionInfos(r.supportedVersions), ChainID: r.chainID, NodeVersion: r.nodeVersion, MinClientVersion: r.minClientVersion, } }Also applies to: 137-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/registry.go` around lines 41 - 44, NewRegistry currently stores the caller-owned slice supportedVersions by reference and ToAkash returns internal slices directly, allowing external mutation; update NewRegistry (and any constructor) to clone the input slice into VersionRegistry.supportedVersions (make a new slice and copy elements) and update ToAkash (and any getters that return SupportedVersions or Modules) to return copies of internal slices/objects instead of the backing arrays so callers cannot mutate internal state outside the mutex; reference NewRegistry, VersionRegistry.supportedVersions, ToAkash, SupportedVersions, and Modules when making these defensive copies.
🧹 Nitpick comments (3)
go/node/client/README.md (3)
47-48: Avoid hardcodingv1beta3in compatibility guidance.Line 48 hardcodes a specific version in prose. Consider phrasing this as an example so the README doesn’t become stale when the oldest-supported version changes.
Proposed wording tweak
-- **Old clients connecting to new nodes** read only `client_info.api_version` - (`v1beta3`), ignore the new fields, and continue working. +- **Old clients connecting to new nodes** read only `client_info.api_version` + (for example, `v1beta3`), ignore the new fields, and continue working.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/README.md` around lines 47 - 48, Update the README sentence that hardcodes "v1beta3": change the phrasing around client_info.api_version to use an example or a placeholder (e.g., "such as v1beta3" or "the oldest-supported version") instead of a fixed literal so the compatibility guidance doesn't become stale; locate the mention of client_info.api_version and replace the hardcoded "`v1beta3`" with example wording or a variable-like phrase.
163-170: Define or removeopts...in this standalone example.At Line 169,
opts...is referenced but not declared in this snippet. This makes copy/paste fail. Either declareoptsin the block or omit it from the call.Proposed doc fix
err := aclient.DiscoverClient(ctx, cctx, func(cl interface{}) error { var ok bool if myClient, ok = cl.(aclient.Client); !ok { return fmt.Errorf("unexpected client type %T", cl) } return nil -}, opts...) +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/README.md` around lines 163 - 170, The example calls aclient.DiscoverClient(..., opts...) but opts is not defined; either remove the trailing opts... from the example call to make the snippet self-contained or declare a minimal opts variable (e.g. a zero-value slice of the appropriate option type) above the DiscoverClient invocation; update the snippet around the DiscoverClient usage so it compiles standalone and references the DiscoverClient and aclient.Client symbols consistently.
62-62: Use hyphenated heading:Server-Side.Line 62 should use the compound adjective form for consistency/readability in docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/node/client/README.md` at line 62, Change the heading text "Server Side" to the hyphenated compound adjective form "Server-Side" in the README; locate the heading line that currently reads "## Server Side" and update it to "## Server-Side" to keep documentation style consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@go/node/client/registry.go`:
- Around line 41-44: NewRegistry currently stores the caller-owned slice
supportedVersions by reference and ToAkash returns internal slices directly,
allowing external mutation; update NewRegistry (and any constructor) to clone
the input slice into VersionRegistry.supportedVersions (make a new slice and
copy elements) and update ToAkash (and any getters that return SupportedVersions
or Modules) to return copies of internal slices/objects instead of the backing
arrays so callers cannot mutate internal state outside the mutex; reference
NewRegistry, VersionRegistry.supportedVersions, ToAkash, SupportedVersions, and
Modules when making these defensive copies.
---
Nitpick comments:
In `@go/node/client/README.md`:
- Around line 47-48: Update the README sentence that hardcodes "v1beta3": change
the phrasing around client_info.api_version to use an example or a placeholder
(e.g., "such as v1beta3" or "the oldest-supported version") instead of a fixed
literal so the compatibility guidance doesn't become stale; locate the mention
of client_info.api_version and replace the hardcoded "`v1beta3`" with example
wording or a variable-like phrase.
- Around line 163-170: The example calls aclient.DiscoverClient(..., opts...)
but opts is not defined; either remove the trailing opts... from the example
call to make the snippet self-contained or declare a minimal opts variable (e.g.
a zero-value slice of the appropriate option type) above the DiscoverClient
invocation; update the snippet around the DiscoverClient usage so it compiles
standalone and references the DiscoverClient and aclient.Client symbols
consistently.
- Line 62: Change the heading text "Server Side" to the hyphenated compound
adjective form "Server-Side" in the README; locate the heading line that
currently reads "## Server Side" and update it to "## Server-Side" to keep
documentation style consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9ed45ca-eb6e-4982-97b5-69e679dffe6f
⛔ Files ignored due to path filters (13)
go/node/client/akash.pb.gois excluded by!**/*.pb.gogo/node/client/client_info.pb.gois excluded by!**/*.pb.gogo/node/client/service.pb.gois excluded by!**/*.pb.gogo/node/client/service.pb.gw.gois excluded by!**/*.pb.gw.gors/src/gen/node/akash.discovery.v1.rsis excluded by!**/gen/**rs/src/gen/node/akash.discovery.v1.serde.rsis excluded by!**/gen/**rs/src/gen/node/akash.discovery.v1.tonic.rsis excluded by!**/gen/**ts/src/generated/createNodeSDK.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/akash.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/client_info.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/service.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/service_akash.tsis excluded by!**/generated/**ts/src/generated/protos/index.akash.v1.tsis excluded by!**/generated/**
📒 Files selected for processing (13)
docs/proto/node.mdgo/node/client/README.mdgo/node/client/client.gogo/node/client/discovery/client.gogo/node/client/discovery_server.gogo/node/client/negotiation_test.gogo/node/client/registry.gogo/node/client/registry_test.gogo/node/client/v1beta4/client.gogo/testutil/cmocks.goproto/node/akash/discovery/v1/akash.protoproto/node/akash/discovery/v1/client_info.protoproto/node/akash/discovery/v1/service.proto
✅ Files skipped from review due to trivial changes (5)
- go/node/client/discovery_server.go
- go/node/client/registry_test.go
- go/node/client/negotiation_test.go
- proto/node/akash/discovery/v1/client_info.proto
- go/node/client/v1beta4/client.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go/testutil/cmocks.go
- proto/node/akash/discovery/v1/akash.proto
Extend the Akash discovery proto (akash.discovery.v1) with supported_versions, chain-id, node version, and per-module version metadata. Add a gRPC Discovery service alongside the existing CometBFT JSON-RPC route. Signed-off-by: Artur Troian <troian@users.noreply.github.com>
troian
force-pushed
the
client-discovery
branch
from
713830d to
d8a3480
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/proto/node.md (1)
3691-3813:⚠️ Potential issue | 🟡 MinorRegenerate the Discovery docs without indented headings.
These new Discovery headings still start with a leading space, so
markdownlintkeeps reporting MD023 on the generated output. Please fix the generator template or suppress the rule for generated docs instead of hand-editing this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proto/node.md` around lines 3691 - 3813, The generated Discovery docs contain headings with a leading space (e.g., " ### ModuleVersion", " ### VersionInfo", " ### Akash", " ### Discovery") which triggers markdownlint MD023; update the generator template that emits those section headings to trim leading whitespace so headings are emitted as "### ..." (or alternatively inject a generated-file disable for MD023 at the top of the produced file), then re-run the generator to produce docs without the leading space.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go/node/client/README.md`:
- Line 62: Change the heading text "## Server Side" to the hyphenated form "##
Server-Side" throughout the README (locate the heading string "## Server Side"
and replace it with "## Server-Side" so the compound adjective is correct).
In `@go/node/client/registry.go`:
- Around line 40-48: NewRegistry accepts versions in arbitrary order but
OldestVersion() and ToAkash() expect the last element to be the legacy fallback;
normalize or validate the slice in NewRegistry so supportedVersions is always
newest→oldest. Inside NewRegistry (and before calling any opts), copy the input
with copyVersions and then sort supportedVersions descending by semantic version
(use sort.Slice with a semantic compare on VersionInfo's version field) or,
alternatively, fail fast if the input is not ordered and require callers to
provide an explicit fallback via a RegistryOption (e.g. WithFallbackVersion) and
document that OldestVersion()/ToAkash() use that fallback; ensure the chosen
approach updates NewRegistry, VersionRegistry.supportedVersions, and any opts
handling to preserve the invariant.
---
Duplicate comments:
In `@docs/proto/node.md`:
- Around line 3691-3813: The generated Discovery docs contain headings with a
leading space (e.g., " ### ModuleVersion", " ### VersionInfo", " ### Akash", "
### Discovery") which triggers markdownlint MD023; update the generator template
that emits those section headings to trim leading whitespace so headings are
emitted as "### ..." (or alternatively inject a generated-file disable for MD023
at the top of the produced file), then re-run the generator to produce docs
without the leading space.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9db02fd7-829a-4954-948b-fa5462b82d4a
⛔ Files ignored due to path filters (16)
go/cli/go.sumis excluded by!**/*.sumgo/go.sumis excluded by!**/*.sumgo/node/client/akash.pb.gois excluded by!**/*.pb.gogo/node/client/client_info.pb.gois excluded by!**/*.pb.gogo/node/client/service.pb.gois excluded by!**/*.pb.gogo/node/client/service.pb.gw.gois excluded by!**/*.pb.gw.gogo/sdl/go.sumis excluded by!**/*.sumrs/src/gen/node/akash.discovery.v1.rsis excluded by!**/gen/**rs/src/gen/node/akash.discovery.v1.serde.rsis excluded by!**/gen/**rs/src/gen/node/akash.discovery.v1.tonic.rsis excluded by!**/gen/**ts/src/generated/createNodeSDK.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/akash.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/client_info.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/service.tsis excluded by!**/generated/**ts/src/generated/protos/akash/discovery/v1/service_akash.tsis excluded by!**/generated/**ts/src/generated/protos/index.akash.v1.tsis excluded by!**/generated/**
📒 Files selected for processing (16)
docs/proto/node.mdgo/cli/go.modgo/go.modgo/node/client/README.mdgo/node/client/client.gogo/node/client/discovery/client.gogo/node/client/discovery_server.gogo/node/client/negotiation_test.gogo/node/client/registry.gogo/node/client/registry_test.gogo/node/client/v1beta4/client.gogo/sdl/go.modgo/testutil/cmocks.goproto/node/akash/discovery/v1/akash.protoproto/node/akash/discovery/v1/client_info.protoproto/node/akash/discovery/v1/service.proto
✅ Files skipped from review due to trivial changes (6)
- go/cli/go.mod
- go/sdl/go.mod
- go/node/client/negotiation_test.go
- proto/node/akash/discovery/v1/client_info.proto
- go/node/client/v1beta4/client.go
- go/node/client/registry_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go/testutil/cmocks.go
- go/node/client/discovery/client.go
| | gRPC | `akash.discovery.v1.Discovery/GetInfo` | Requires explicit `RegisterDiscoveryService` call on the gRPC server. | | ||
| | REST | `GET /akash/discovery/v1/info` | gRPC-Gateway route; requires `RegisterDiscoveryHandlerServer`. | | ||
|
|
||
| ## Server Side |
Contributor
There was a problem hiding this comment.
Use hyphenation in the section heading.
Please change ## Server Side to ## Server-Side for correct compound-adjective usage.
🧰 Tools
🪛 LanguageTool
[grammar] ~62-~62: Use a hyphen to join words.
Context: ...overyHandlerServer`. | ## Server Side ### 1. Configure the Registry Cre...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go/node/client/README.md` at line 62, Change the heading text "## Server
Side" to the hyphenated form "## Server-Side" throughout the README (locate the
heading string "## Server Side" and replace it with "## Server-Side" so the
compound adjective is correct).
Comment on lines
+40
to
+48
| // NewRegistry creates a new VersionRegistry with the given supported versions. | ||
| func NewRegistry(versions []VersionInfo, opts ...RegistryOption) *VersionRegistry { | ||
| r := &VersionRegistry{ | ||
| supportedVersions: copyVersions(versions), | ||
| } | ||
| for _, opt := range opts { | ||
| opt(r) | ||
| } | ||
| return r |
Contributor
There was a problem hiding this comment.
Validate or normalize versions order in NewRegistry.
OldestVersion() and ToAkash() both assume the last slice element is the legacy fallback version. Right now NewRegistry accepts arbitrary input order, so a custom registry can silently advertise the wrong client_info.api_version just by passing an unsorted slice. Please enforce the newest→oldest invariant here, or make the legacy fallback explicit instead of deriving it from position.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go/node/client/registry.go` around lines 40 - 48, NewRegistry accepts
versions in arbitrary order but OldestVersion() and ToAkash() expect the last
element to be the legacy fallback; normalize or validate the slice in
NewRegistry so supportedVersions is always newest→oldest. Inside NewRegistry
(and before calling any opts), copy the input with copyVersions and then sort
supportedVersions descending by semantic version (use sort.Slice with a semantic
compare on VersionInfo's version field) or, alternatively, fail fast if the
input is not ordered and require callers to provide an explicit fallback via a
RegistryOption (e.g. WithFallbackVersion) and document that
OldestVersion()/ToAkash() use that fallback; ensure the chosen approach updates
NewRegistry, VersionRegistry.supportedVersions, and any opts handling to
preserve the invariant.
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
1 participant
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.
Extend the Akash discovery proto (akash.discovery.v1) with supported_versions, chain-id, node version, and per-module version metadata. Add a gRPC Discovery service alongside the existing CometBFT JSON-RPC route.
📝 Description
[Explain what this PR does in 2-3 sentences. Include context about the feature or problem being solved]
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
📎 Notes for Reviewers
[Include any additional context, architectural decisions, or specific areas to focus on]