feat: automatically determine max supported module API version#8461
feat: automatically determine max supported module API version#8461FrankChinedu wants to merge 1 commit into
Conversation
fedimint-bot
left a comment
There was a problem hiding this comment.
Summary
This PR eliminates manually maintained supported_api_versions() implementations from all server-side module init types. Instead, it derives the supported API versions automatically at startup by inspecting the VERSION constant on each registered ApiEndpoint and computing the maximum minor version per major group. The ApiEndpoint struct gains a version field, the TypedApiEndpoint trait gains VERSION: ApiVersion, and supported_api_versions_summary is updated to accept the live ModuleRegistry<DynServerModule> rather than the init registry.
Consensus Impact
The change does not touch transaction processing, encoding, BFT consensus, or session finalization. Its impact is confined to API version advertisement: what SupportedApiVersionsSummary the server reports to clients during version negotiation. However, a discrepancy has been found: fedimint-walletv2-server previously declared (0, 1) as its maximum supported API version, but all nine of its api_endpoint! calls annotate with ApiVersion::new(0, 0). Under the new code the server will advertise (0, 0) instead, which is a regression. Similarly, fedimint-unknown-server and fedimint-empty-server expose no endpoints at all; the derived MultiApiVersion will be empty rather than (0, 0). Whether clients are affected depends on how they handle an empty version list, but the behavioral change is unintentional.
Review Summary
The approach is sound: deriving supported API versions from the endpoints that are actually registered avoids the classic drift problem where the hardcoded declaration diverges from reality. The macro refactor cleanly moves the version constant from a dead-code local to a proper trait constant.
Critical issue — walletv2 version regression: All endpoints in fedimint-walletv2-server are annotated ApiVersion::new(0, 0), but the removed code advertised (0, 1). The new logic will advertise (0, 0). This is a silent behavioural downgrade for any client using that module.
Edge case — empty-endpoint modules: fedimint-unknown-server and fedimint-empty-server return Vec::new() from api_endpoints(), so the derived MultiApiVersion is empty. The old code returned (0, 0). Downstream behaviour when a client encounters an empty version list should be confirmed.
mintv2 needs verification: The removed code advertised (0, 1) for mintv2 but the endpoint annotations were not checked as part of this diff. Please confirm at least one endpoint carries ApiVersion::new(0, 1).
Once the walletv2 annotation discrepancy is resolved (either fix the endpoint annotations or document the intentional downgrade) and the empty-module behaviour is addressed, the overall design is a clear improvement over the status quo.
Auto-approval is disabled for PRs touching consensus-critical code. A human reviewer must approve this PR.
| MODULE_CONSENSUS_VERSION.minor, | ||
| ), | ||
| &[(0, 1)], | ||
| ) |
There was a problem hiding this comment.
Critical: All nine api_endpoint! calls in this file use ApiVersion::new(0, 0) as $version_introduced. The new auto-derivation logic computes max(minor) across all endpoints, so it will produce (0, 0) — not the (0, 1) this block previously advertised. Any client that negotiated (0, 1) features for walletv2 will now see the server report a lower version. Either the endpoint annotations need to be bumped to (0, 1) for the endpoints that introduced that version, or the old (0, 1) was incorrect and this needs a protocol-level note about the intentional downgrade.
| MODULE_CONSENSUS_VERSION.minor, | ||
| ), | ||
| &[(0, 1)], | ||
| ) |
There was a problem hiding this comment.
Warning: This module previously advertised (0, 1). Please verify that at least one api_endpoint! call in MintV2Init::api_endpoints() is annotated with ApiVersion::new(0, 1) (or higher). If all endpoints still carry ApiVersion::new(0, 0) the derived maximum will be (0, 0), silently downgrading the advertised version.
|
Please use |
|
I see 3 commits still. git rebase, then squash all into one. Other than that LGTM. |
elsirion
left a comment
There was a problem hiding this comment.
Ack, just as DPC said: please squash
f86c133 to
f161c46
Compare
Head branch was pushed to by a user without write access
4a3c52e to
174af47
Compare
174af47 to
471d4e5
Compare
Instead of requiring each ServerModuleInit to manually declare its
supported API versions (which is easy to forget when adding new
endpoints), derive them automatically from the module's api_endpoints().
The version introduced in each api_endpoint! macro call is now stored
on ApiEndpoint as a
versionfield (previously it was only used as acompile-time type check). supported_api_versions_summary now iterates
the live module's endpoints and computes max(minor) per major version.
Removes the supported_api_versions() method from ServerModuleInit and
IServerModuleInit traits, and its manual implementation from all 10
server modules.
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com"
Closes #6650