Skip to content

Add VSS Http thin client implementation for get/put/listKeyVersions api's #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 20, 2023

Conversation

G8XSU
Copy link
Collaborator

@G8XSU G8XSU commented Apr 13, 2023

Depends on #2

  • Tests to be added in next commit/pr

@G8XSU G8XSU requested a review from jkczyz April 13, 2023 09:13
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick initial pass mostly looking at the project setup, not the code itself yet.

We may want to add a rustfmt.toml and a simple CI enforcing cargo fmt / cargo build / cargo test from the start. Feel free to simply copy these files over from LDK Node to start out:

https://github.com/lightningdevkit/ldk-node/blob/6aa4a862b4299d2549689beb4b8d231b44f681b5/rustfmt.toml

https://github.com/lightningdevkit/ldk-node/blob/6aa4a862b4299d2549689beb4b8d231b44f681b5/.github/workflows/build.yml

Cargo.toml Outdated
@@ -0,0 +1,4 @@
[workspace]
members = [
"vss-accessor",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which other members will be added here? Do they need to be in separate crates, or can they live in submodules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there will be an additional vss-lib added in future (mainly phase-2) for abstracting out additional versioning handling.
we want to give flexibility to clients to just depend on the basic thin client instead of full lib.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. Not sure if the library size warrants the additional complexity of splitting it up in sub-crates, but I generally have no strong opinion on the matter.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a single crate. When you start breaking across crates you lose the ability to use pub(crate)/pub(super) and make it difficult to implement traits on external types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fat lib of vss could have significantly more dependencies.
We might need to depend on some in-memory/file-based database (sled/sqlite) to implement SecondaryKVStore.
That will help with maintaining global-version and sync across multiple-devices.
For single device-use it is very simple to just directly use vss-accessor (thin-client). Hence the separation.

regarding pub(crate), there is separation of concern between vss-thin-client and fat-lib, we shouldn't be needing it ideally.

@G8XSU
Copy link
Collaborator Author

G8XSU commented Apr 19, 2023

Will add CI workflow and fmt in separate PR.

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the idea for testing this? Will we be able to run the VSS Server in CI and write integration tests again it?

let response = GetObjectResponse::decode(&payload[..])?;
Ok(response)
} else {
let error_response = ErrorResponse::decode(&payload[..])?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log the status code in these cases? Also, can we log the error responses in a more human-readable manner than just returning raw bytes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifying :

  • we are not returning raw bytes, we deserialize byte response into error_response struct. it has message and error code for now, client can use that directly if they want.
  • logging/metrics - it doesn't belong in this thin-client layer, should be implemented in application layer consuming this thin-client. for e.g. Client-1 want to just use vss-accessor without any logs, they have there own custom log msgs and metrics.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, so the error code is/does cover the same error cases as the HTTP status code? Or do we lose information by not propagating the status here? I agree that logging should happen somewhere else, but if we don't do any logging here we need to make sure to propagate ample debug information to the next layer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrorCode is more granular, from error_response perspective we do not lose any information here.
I agree that we should provide necessary information to debug for next layer.

@G8XSU
Copy link
Collaborator Author

G8XSU commented Apr 24, 2023

For testing, we will have units tests for this module using mockito server, which ensures there is correct forming of request and response deserialization. Will not be testing exact logic of vss-server or its functionality, as that is already tested on server side.

In this, we just to test/verify correct request/response formation and correctly making POST http requests on endpoint.

@tnull
Copy link
Contributor

tnull commented Apr 25, 2023

For testing, we will have units tests for this module using mockito server, which ensures there is correct forming of request and response deserialization. Will not be testing exact logic of vss-server or its functionality, as that is already tested on server side.

In this, we just to test/verify correct request/response formation and correctly making POST http requests on endpoint.

Mh, but how do you verify that the mocked behavior actually reflects the server behavior? If we don't close that gap we'd only ever test that the client can work against the mock model, so basically not doing any actual integration testing for the client at all?

@G8XSU
Copy link
Collaborator Author

G8XSU commented Apr 25, 2023

We will have some level of integration testing as well to verify client & server are able to communicate as expected but i am not sure if it should verify full-server functionality.
Server logic will be verified and tested by server-side integration tests.

@G8XSU G8XSU force-pushed the vss-accessor-impl branch 3 times, most recently from abcd259 to e909549 Compare April 26, 2023 20:46
This was referenced Apr 26, 2023
@G8XSU G8XSU force-pushed the vss-accessor-impl branch from e909549 to fff2d52 Compare April 27, 2023 17:13
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When trying to compile this (or any dependent PR) locally, I get the error:

error: failed to run custom build command for `vss-accessor v0.1.0 (/Users/ero/workspace/vss-rust-client/vss-accessor)`

Caused by:
  process didn't exit successfully: `/Users/ero/workspace/vss-rust-client/target/debug/build/vss-accessor-3a0e3dbef1470007/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'Could not find `protoc` installation and this build crate cannot proceed without
      this knowledge. If `protoc` is installed and this crate had trouble finding
      it, you can set the `PROTOC` environment variable with the specific path to your
      installed `protoc` binary.You could try running `brew install protobuf` or downloading it from https://github.com/protocolbuffers/protobuf/releases

  For more information: https://docs.rs/prost-build/#sourcing-protoc
  ', /Users/ero/.cargo/registry/src/github.com-1ecc6299db9ec823/prost-build-0.11.9/src/lib.rs:1457:10
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
exit 101

Can we add instructions for how to acquire the protobuf dependency to the README?

@G8XSU
Copy link
Collaborator Author

G8XSU commented May 2, 2023

Hmm.. that error should be self-explanatory ?
it says "For more information: https://docs.rs/prost-build/#sourcing-protoc"
It is not something directly being used by us but by prost_build.

But, i will keep a note of it to add it in README as dependencies to be installed.
And link to https://grpc.io/docs/protoc-installation/

@tnull
Copy link
Contributor

tnull commented May 3, 2023

Hmm.. that error should be self-explanatory ? it says "For more information: https://docs.rs/prost-build/#sourcing-protoc" It is not something directly being used by us but by prost_build.

But, i will keep a note of it to add it in README as dependencies to be installed. And link to https://grpc.io/docs/protoc-installation/

Yes, it kind of is self-explanatory. Still we should mention the dependency, give install instructions, and make sure it actually builds in CI.

Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add rustfmt now rather than later to avoid churn in the blame history.

Cargo.toml Outdated
@@ -0,0 +1,4 @@
[workspace]
members = [
"vss-accessor",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a single crate. When you start breaking across crates you lose the ability to use pub(crate)/pub(super) and make it difficult to implement traits on external types.

@G8XSU G8XSU force-pushed the vss-accessor-impl branch 2 times, most recently from 8fb5f5c to 5d70ce6 Compare May 4, 2023 03:00
@G8XSU
Copy link
Collaborator Author

G8XSU commented May 4, 2023

Now this PR depends on #2

@G8XSU G8XSU force-pushed the vss-accessor-impl branch 2 times, most recently from d881c08 to 1bd7c47 Compare May 4, 2023 23:31
@G8XSU G8XSU force-pushed the vss-accessor-impl branch from 1bd7c47 to 4e74eab Compare May 12, 2023 21:11
@G8XSU G8XSU force-pushed the vss-accessor-impl branch 2 times, most recently from 3e85880 to 79829db Compare July 11, 2023 22:06
@G8XSU G8XSU requested a review from jkczyz July 12, 2023 07:09
@tnull
Copy link
Contributor

tnull commented Jul 12, 2023

This needs a rebase now that #2 has been merged.

@G8XSU G8XSU force-pushed the vss-accessor-impl branch 2 times, most recently from 751583f to fa3c4eb Compare July 12, 2023 18:42
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick pass on VssClient. I mainly highlighted doc nits, but the more important question is whether we shouldn't construct the request/response objects for the user rather having them figure out how to construct them and, for example, what store_id or global_version to use?

/// Fetches a value against a given `key` in `request`.
/// Makes a service call to GetObject endpoint of vss-server.
/// For api-contract/usage, refer docs for: [`GetObjectRequest`] and [`GetObjectResponse`]
pub async fn get_object(&self, request: GetObjectRequest) -> Result<GetObjectResponse, VssError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these calls take the relevant data (key in this case) directly? I.e., should we create the GetObjectRequest for the users rather than having them figure out how to construct it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i considered that, that was the api signature earlier.
reason we moved to current approach is for vss-phase-2.
in phase-2, we will have a local secondary storage which will act on these same requests and perform similar operations. hence having this request object and handing it to both primary and secondary storage made more sense.
request creation isn't that complex and we can abstract it at higher layer if needed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't you need to take request by reference then? Otherwise, the value would be moved here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i considered that, that was the api signature earlier. reason we moved to current approach is for vss-phase-2. in phase-2, we will have a local secondary storage which will act on these same requests and perform similar operations. hence having this request object and handing it to both primary and secondary storage made more sense. request creation isn't that complex and we can abstract it at higher layer if needed.

Ah, that makes sense to me. How would we envision the user interact with both stores? Wouldn't we expect VSSClient to wrap and manage both, primary and secondary storage, internally and query them as needed? Or would we then provide yet another wrapper object that queries the local cache storage and uses this VSSClient to query the server?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be another wrapper which handles primary secondary and sync b/w them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had named this accessor and was going to name that wrapper as VssClient but now we will have another name for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don't expect this to be the final interface for the user? Does all of this then even need to be public, or can it be private if it's going to be wrapped in a more user-friendly abstraction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the final interfaces for user,
user doesn't have to use Primary-Secondary storage. they should just be able to use vss client directly or write some other wrapper if required.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, this is fine as VssClient and any other classes composing or decorating can be named accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, in particular if the (multiplayer) interface will take care of most of the rough edges here.

@G8XSU G8XSU force-pushed the vss-accessor-impl branch 2 times, most recently from 91e9767 to 8361f7f Compare July 12, 2023 20:25
include!("generated-src/org.vss.rs");
}

/// Thin-client to access hosted instance of `Versioned Storage Service (VSS)`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need ticks here since it is not an identifier.

Copy link
Collaborator Author

@G8XSU G8XSU Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i understand `[ ]` is to be used for identifiers,
there's nothing specific about `` right ? its just for additional emphasize or highlight.
e.g: https://doc.rust-lang.org/beta/src/std/lib.rs.html#38
let me know if its something else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i understand [ ] is to be used for identifiers, there's nothing specific about `` right ? its just for additional emphasize or highlight. e.g: https://doc.rust-lang.org/beta/src/std/lib.rs.html#38 let me know if its something else.

Mh, it's probably not that important, but not exactly. Usually backticks are used to typeset identifiers or code snippets while emphases and highlights are set via asterisks or underscores (see the CommonMark Spec upon which rustdoc is based.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, as @tnull said for backticks. The square brackets are for linking, typically to something in scope. See:

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update package name and directory. Can wait until the end of the review the preserve discussion comments.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current code looks good. As for project layout, I think we should discuss what this will look like as higher-level abstractions are added. This may dictate whether VssClient should be in a separate module rather than directly in lib.rs.

@G8XSU Could you briefly list what other abstractions we'll likely have in the future?

@tnull Curious about your thoughts on this as I know ldk-node had became more modularized over time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current code looks good. As for project layout, I think we should discuss what this will look like as higher-level abstractions are added. This may dictate whether VssClient should be in a separate module rather than directly in lib.rs.

@G8XSU Could you briefly list what other abstractions we'll likely have in the future?

I agree. Also given the interface discussion above, I think it might be useful to map out what we expect the final version at end of phase 2 will look like from a user's perspective.

@tnull Curious about your thoughts on this as I know ldk-node had became more modularized over time.

Given that the client side is really not holding that much logic, I still think we might not need to break it into different crates at all and could possibly get away with keeping the exposed API limited to a small number of user-facing objects that the user can control via config knobs, or, if required, a builder pattern. I'd honestly even prefer to have a single user-facing object but, considering the discussion above, having one for single-player and one for multi-player mode should be fine. However, at that point we should try to keep their API very similar, which would then probably mean creating two new objects that both internally wrap a VssClient.

Btw., I assume we plan to allow the user to upgrade from single- to multi-player VSS? Are there any particular steps the user would need to take (besides adding the node restart logic), or would it on the client side really just be a matter of start using the 'multiplayer client' with the previous config and be done with it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see #1 (comment):

If we were to get rid of the crate structure, I'd argue we would want to include the generated objects as part of a client sub-module also holding VssClient. If we were to keep the workspace approach, we still might want to get rid of the vss sub-module and just have the imported objects live side-by-side with the VssClient that uses them?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may ultimately have one interface regardless of mode. I'm indifferent at the moment but would be curious what @G8XSU thought.

Otherwise, I think we want one crate but possibly multiple modules. Though I'm thinking any submodules may just be for internal organization if we have a single user-facing interface. I ask now just to avoid needing to move this code to another file later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the package name and directory to reflect vss-client and for now and only have single crate.

i think from client-perspective it makes sense to have multiple entry-points, i wouldn't want to go through all the complexity of multi-device if i am only interested in single-device and should be able to pick and choose things individually if some components need minor modification acc. to my specific needs.

I think vss-client can be in separate file/mod, since we plan on having multiple structs with use. At the same time i am not worried about moving code as it is, down the lane if needed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also answer @tnull's questions above (#1 (comment)) and address his comment (#1 (comment))? Otherwise, feel free to make the necessary changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we plan to allow the user to upgrade from single- to multi-player VSS? Are there any particular steps the user would need to take (besides adding the node restart logic), or would it on the client side really just be a matter of start using the 'multiplayer client' with the previous config and be done with it?

Yes client should have an upgrade path from single to multi-device support,
Client would need to do the following in order to do so:

  • Switch to PrimarySecondary Storage instead of vss only. (This should be backwards compatible)
  • Add restart logic in node. Before restarting, call the sync() function from PrimarySecondaryVssStorage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes client should have an upgrade path from single to multi-device support, Client would need to do the following in order to do so:

  • Switch to PrimarySecondary Storage instead of vss only. (This should be backwards compatible)
  • Add restart logic in node. Before restarting, call the sync() function from PrimarySecondaryVssStorage

Alright, that should work! Still think it might be preferable to rather make this a config knob on a single-object interface rather than having multiple separate objects with different APIs, but it's probably not that much pain for the user. We only need to assert that the two versions really maintain compatible over time and we'll have to see how we can minimize duplicate code between the two impls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VssClient will be injected into PrimarySecondaryStorage, so there will be no duplicate code.
API for those two might slightly differ, though.
Main motivation for doing so is: there are multiple ways to do multi-device, one of them being PrimarySecondary, there are more layers of abstraction possible, for e.g. dual/multi-write (multiple vss servers)

/// Fetches a value against a given `key` in `request`.
/// Makes a service call to GetObject endpoint of vss-server.
/// For api-contract/usage, refer docs for: [`GetObjectRequest`] and [`GetObjectResponse`]
pub async fn get_object(&self, request: GetObjectRequest) -> Result<GetObjectResponse, VssError> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't you need to take request by reference then? Otherwise, the value would be moved here.

Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I unresolved some comments that were not addressed without any explanation.

include!("generated-src/org.vss.rs");
}

/// Thin-client to access hosted instance of `Versioned Storage Service (VSS)`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, as @tnull said for backticks. The square brackets are for linking, typically to something in scope. See:

let response = GetObjectResponse::decode(&payload[..])?;
Ok(response)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why use a variable here but not for the error case?

@G8XSU
Copy link
Collaborator Author

G8XSU commented Jul 13, 2023

@jkczyz i resolved comments earlier as i was fixing them locally, await the minor fix commit :)

@G8XSU G8XSU force-pushed the vss-accessor-impl branch 2 times, most recently from 193bffc to 6259398 Compare July 13, 2023 17:52
@G8XSU G8XSU requested a review from jkczyz July 13, 2023 22:05
@jkczyz
Copy link

jkczyz commented Jul 14, 2023

@jkczyz i resolved comments earlier as i was fixing them locally, await the minor fix commit :)

Hmm... If you push a commit and reply to / resolve comments, I'd expect the push to reflect this. Could you start using fixup commits? It would be easier to see which comments have been addressed in the latest push that way.


mod vss_error;

/// Import auto-generated code related to request/response objects of VSS
Copy link
Contributor

@tnull tnull Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the rendered docs, this is unfortunately still not very helpful. What does 'related' mean?

If the user needs to use these objects in conjunction with VssClient, would it make sense to move them all together, either all to lib.rs (if we were to keep the sub-crate structure) or to a client module in a single-crate design?

Copy link
Collaborator Author

@G8XSU G8XSU Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change comment to indicate that these are auto-generated objects from protobuf vss specification.
Feel free to suggest anything better.

I don't think we should move/mix in auto-generated code, it should live as it is right now, in a separate mod.
Why it shouldn't live with just vss-client is because at other places we intend to use it, for example in PrimarySecondaryStorage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to types.rs mod within vss-client,
So imports will look like vss-client::types::*
I think that looks cleaner.

Copy link
Contributor

@tnull tnull Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should move/mix in auto-generated code, it should live as it is right now, in a separate mod. Why it shouldn't live with just vss-client is because at other places we intend to use it, for example in PrimarySecondaryStorage.

I guess having them in types.rs is fine, especially now that we moved to a single-crate layout. The name could be better tbh, but I ended up being just as uncreative when having this issue in LDK Node (see types.rs there 😆). (If at all) we may want to consider exposing them from the crate root though.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about messages.rs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it is just messages, but vss proto definition might evolve to contain some other helper objects,
Hence the types.rs, i think it aligns nicely with rust's notation of calling them types as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it is just messages, but vss proto definition might evolve to contain some other helper objects, Hence the types.rs,

Well, everything is a type lol. That's why we were looking for a more descriptive name.

i think it aligns nicely with rust's notation of calling them types as well.

I don't understand what you mean by this.

@G8XSU G8XSU force-pushed the vss-accessor-impl branch from 52046ab to 1c484fc Compare July 19, 2023 20:59
@G8XSU
Copy link
Collaborator Author

G8XSU commented Jul 19, 2023

Squashing changes

@G8XSU G8XSU force-pushed the vss-accessor-impl branch from 1c484fc to bf39716 Compare July 19, 2023 21:01
Comment on lines 33 to 42
let raw_response = self.client.post(url).body(request.encode_to_vec()).send().await?;
let status = raw_response.status();
let payload = raw_response.bytes().await?;

if status.is_success() {
let response = GetObjectResponse::decode(&payload[..])?;
Ok(response)
} else {
Err(VssError::new(status, payload))
}
Copy link

@jkczyz jkczyz Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the plan for testing?

@tnull I see there is: https://docs.rs/reqwest_mock/latest/reqwest_mock/. Is there a preferred way of going about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing is covered in #3
we create a mock server, and play REST requests against it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For an initial round of tests the mocking approach of #3 should be fine, but I think longer-term it would be nice to have some integration tests running against the actual server code. Possibly after we integrated this with LDK Node we could implement some end-to-end tests based off an actual VSS server backend in CI?

@G8XSU G8XSU force-pushed the vss-accessor-impl branch 2 times, most recently from 8f177c7 to b78cd79 Compare July 19, 2023 22:24
@G8XSU G8XSU force-pushed the vss-accessor-impl branch from 9f26766 to 0031a6b Compare July 20, 2023 01:42
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM.

We def. need more crate-level/module-level docs, but I'm fine to have that happen as a follow-up. I think we also haven't fully figured out the project layout and how what will exposed how exactly to the user, but that can also still change and grow before we do an initial release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're definitely missing some crate-level and module-level docs here and below. Also an initial usage example would be nice-to-have on the landing page. Currently this is just blank:
Screenshot 2023-07-20 at 10 20 46

(Still think we should enable the missing_docs lint from the start)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's add that now. Didn't realize that needed to be explicit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i understand, module docs just need 1-liner description, will add that.

As for usage doc at crate level, i think i would add it pre-release as we grow and add rest of the things.

Let me know if this sounds good:
client - Implements a thin-client[VssClient] to access a hosted instance of Versioned Storage Service (VSS).
error - Implements the error type[VssError] returned on interacting with [VSSClient]
types - Contains request/response types generated from API definition of VSS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now #6

@@ -1,2 +1,6 @@
#![deny(rustdoc::broken_intra_doc_links)]
#![deny(rustdoc::private_intra_doc_links)]

pub mod client;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending of how many objects we expect to expose overall, we might want to consider exposing some part of the (important) objects from the crate root.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case maybe client should have been in lib ? (contrary to our previous discussion to move it out of that)
Let me know if its something else.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always re-export in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i think re-exporting makes sense 👍

@G8XSU G8XSU merged commit a8432f5 into lightningdevkit:main Jul 20, 2023
G8XSU added a commit to G8XSU/vss-rust-client that referenced this pull request Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants