Add VssClient::get_version#61
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
One comment for now, please feel free to update and re-request review once we discussed the general approach on vss-server.
| #[cfg(genproto)] | ||
| fn generate_protos() { | ||
| download_file( | ||
| "https://raw.githubusercontent.com/lightningdevkit/vss-server/022ee5e92debb60516438af0a369966495bfe595/proto/vss.proto", |
There was a problem hiding this comment.
While doing this on the server is fine, here I'd not want to do it until we have a better solution in place. Maybe we should go through with merging repos now that both are rust codebases, or have both depend on a shared vss-proto crate, to ensure version compat?
We now read `vss.proto` from the local repository instead of downloading it from a specific commit on github. We will use protocol versioning, tags, and releases to coordinate clients and servers.
tankyleo
force-pushed
the
2026-05-get-version
branch
2 times, most recently
from
fdc0eb2 to
c8d01d8
Compare
We assert that the VSS protocol version matches on all responses returned from the server, including errors.
tankyleo
force-pushed
the
2026-05-get-version
branch
from
c8d01d8 to
c55fa29
Compare
| f, | ||
| "The VSS server and client speak different versions of the \ | ||
| VSS protocol, the server serves version {}, client expects \ | ||
| {}", |
There was a problem hiding this comment.
Hmm, do we need to sanitize this string before printing or bubbling up the error, or are we expecting this to happen at a higher level anyways?
There was a problem hiding this comment.
hmm thank you yes, I'm thinking we keep validation here minimal and just report the mismatch, together with the different header values ? So as to keep Client as lean as possible.
|
|
||
| let response = self.client.send_async(http_request).await?; | ||
| // Return early in case of version mismatch, this issue must be solved first. | ||
| if response.headers.get(PROTOCOL_VERSION_HEADER).map(String::as_str) |
There was a problem hiding this comment.
Do we need / want to maintain backwards compatibility, i.e., interpret absence of the versioning field as version 0? Or intentionally not?
There was a problem hiding this comment.
If we remain backwards compatible, then we could break the upcoming PaginatedKVStoreAPI, and serve keys not in creation order.
That is why I think the VSS client should reject any VSS servers that don't set the version header.
There was a problem hiding this comment.
I'll sleep on it, strict equality may be too restrictive. That being said in the future, we could ship version 2 in vss-server on a different set of endpoints from the existing /getObject etc..., so that a single instances may serve clients on both versions at the same time.
3 participants
This new method gets the version of the VSS API served by the server. The method depends on the
/versionendpoint recently added to the VSS API.If this method is called against a VSS server that does not serve this endpoint, a
VssError::InternalErrorwill be returned. Upstream libraries should then communicate to users that the VSS server they are talking to is not supported.