Introduce header provider trait #31

wvanlint · 2024-05-15T21:54:54Z

Introduces a HeaderProvider trait that will provide headers for each VSS call.

This change is split off from #26, which will introduce a JWT header provider based on LNURL Auth.

G8XSU · 2024-05-15T22:04:56Z

src/headers/mod.rs

+pub trait VssHeaderProvider {
+	/// Returns the HTTP headers to be used for a VSS request.
+	/// This method is called on each request, and should likely perform some form of caching.
+	async fn get_headers(&self, request: &[u8]) -> Result<HashMap<String, String>, VssError>;


++
request: A reference to serialized request body. It can be used to perform operations such as request signing.

G8XSU · 2024-05-15T22:29:07Z

src/headers/mod.rs

+
+#[async_trait]
+impl VssHeaderProvider for FixedHeaders {
+	async fn get_headers(&self, _request: &[u8]) -> Result<HashMap<String, String>, VssError> {


returning some of the errors in VssError can have unintended consequences such as NoSuchKey/Conflict, or in some cases misleading.

the alternative is, we use io::Result<HashMap<String,String>> here and blanket convert all errors to VssError::AuthError in VssClient. (i think this would be much easier to debug and easy to understand from api perspective)

or we could document that only

InvalidRequestError, (invalid request by user or invalid input) AuthError, (in case of auth failure) (or blanket convert all errors to authError) InternalServerError, (auth server unavailable) InternalError (unexpected code failure) (if the failure could be because of user provided input, it should still be invalid request, for example an error due to user provided headers or seed)

should be used here and explain how they will be used for auth_provider.

Yeah I agree, this was the main reason I initially created a separate error type. Moved to io::Result which will be embedded in VssError::AuthError.

sounds good,
I was also good with the way we have done error handling in lnurlauthprovider, it would just need some explaining in trait docs.

I'm afraid if we want to expose/use this interface in LDK Node bindings, we need to revert the recent changes as discussed in #26. We need to keep the simpler error types as io::Error is probably not feasible to expose in bindings.

Ah right, I forgot whether this will turn out internal in ldk-node or exposed as bindings. We did discuss the option of having this exposed as bindings, so I changed this into Result<_, String> to not have different semantics around VssError for different cases.

Ah right, I forgot whether this will turn out internal in ldk-node or exposed as bindings. We did discuss the option of having this exposed as bindings, so I changed this into Result<_, String> to not have different semantics around VssError for different cases.

Unfortunately the error type must be an enum and implement std::error::Error (see https://mozilla.github.io/uniffi-rs/udl/errors.html), so I think we really have to revert to what we had discussed before.

ok, in that case lets just revert to using VssError, we are using it correctly in lnurl PR.
and have some docs in this trait as discussed in first comment.

Ah, thanks for mentioning that! I should have double-checked UniFFI. Reverted back to VssError and added documentation in the trait. Enforced all VssHeaderProvider errors to be a single variant i.e. VssError::AuthError to not conflate semantics.

ok, in that case lets just revert to using VssError, we are using it correctly in lnurl PR. and have some docs in this trait as discussed in first comment.

No, I really meant it, we should revert to the simple error type: UniFFI doesn't support tuple structs/enums. If we want to use VssError, it can't have any such variants, i.e., VssError::AuthError(String) would need to become AuthError(error: String) etc. It also shouldn't rely on any constructors/methods, as otherwise needs to be exposed differently.

Reverting back to what we previously discussed seems much simpler.

Reverted back to the original UniFFI compatible error type previous to #26 (comment).

Cargo.lock

G8XSU · 2024-05-15T22:33:21Z

src/client.rs

+			.get_headers(&request_body)
+			.await
+			.map_err(|e| VssError::InternalError(e.to_string()))?;
+		let headermap = get_headermap(&headers).map_err(VssError::InternalError)?;


if we can't get string value header map from headers, then there is probably some user provided invalid input.
i think this can be InvalidRequestError.

Similar to the discussion above, using io::Error that will be embedded in VssError::AuthError. These might also be invalid headers by the provider.

wvanlint · 2024-05-22T23:24:58Z

FYI I am aiming to get this merged first before rebasing #26 if that sounds good.

src/headers/mod.rs

src/client.rs

G8XSU · 2024-05-23T23:24:09Z

src/client.rs

+			.get_headers(&request_body)
+			.await
+			.and_then(get_headermap)
+			.map_err(|e| match e {


should we add a simple testcase for this ?

tnull · 2024-05-24T06:38:05Z

src/client.rs

 	}

 	/// Constructs a [`VssClient`] from a given [`reqwest::Client`], using `base_url` as the VSS server endpoint.
 	pub fn from_client(base_url: &str, client: Client, retry_policy: R) -> Self {


nit: If we end up owning the value anyways, we could consider just taking a String here and below.

src/headers/mod.rs

tnull · 2024-05-24T06:47:36Z

src/headers/mod.rs

+
+#[async_trait]
+impl VssHeaderProvider for FixedHeaders {
+	async fn get_headers(&self, _request: &[u8]) -> Result<HashMap<String, String>, VssError> {


ok, in that case lets just revert to using VssError, we are using it correctly in lnurl PR. and have some docs in this trait as discussed in first comment.

No, I really meant it, we should revert to the simple error type: UniFFI doesn't support tuple structs/enums. If we want to use VssError, it can't have any such variants, i.e., VssError::AuthError(String) would need to become AuthError(error: String) etc. It also shouldn't rely on any constructors/methods, as otherwise needs to be exposed differently.

Reverting back to what we previously discussed seems much simpler.

tnull

LGTM

Just two tiny nits, but happy to have this land as is.

tnull · 2024-07-12T09:39:27Z

src/client.rs

+use std::sync::Arc;

 use crate::error::VssError;
+use crate::headers::get_headermap;


nit: Could group these imports.

tnull · 2024-07-12T09:40:36Z

src/client.rs

+	}
+
+	/// Constructs a [`VssClient`] using `base_url` as the VSS server endpoint.
+	/// HTTP headers will be provided by the given `header_provider`.


nit: Add newline after initial paragraph to improve doc rendering.

Suggested change

/// HTTP headers will be provided by the given `header_provider`.

///

/// HTTP headers will be provided by the given `header_provider`.

G8XSU

Lgtm! Feel free to squash.

wvanlint · 2024-07-17T21:06:20Z

Thanks! Squashed.

Major Changes include: * Signature change in vss-client constructor. (in lightningdevkit#31 ) * Vss-client can now also return AuthError if AuthException is returned from server. (lightningdevkit#30) * Adds VssHeaderProvider, can be used for auth and request signing.(lightningdevkit#31) * Adds LnurlAuthToJwtProvider, provides LnUrl based JWT auth. (lightningdevkit#26) * Adds KeyObfuscator, to provide client-side key obfuscation. (lightningdevkit#32) * Package now has enforced MSRV of 1.63.0. (lightningdevkit#19) This is a minor version bump because there are non-backward compatible changes in vss-client usage.

wvanlint mentioned this pull request May 15, 2024

JWT authorization header based on LNURL Auth #26

Merged

G8XSU reviewed May 15, 2024

View reviewed changes

wvanlint force-pushed the header_provider branch 2 times, most recently from c2d1d00 to 9e151a4 Compare May 16, 2024 23:13

wvanlint requested a review from G8XSU May 16, 2024 23:17

G8XSU approved these changes May 16, 2024

View reviewed changes

wvanlint force-pushed the header_provider branch from 9e151a4 to 92cc59d Compare May 22, 2024 23:05

wvanlint requested review from G8XSU and tnull May 22, 2024 23:06

G8XSU approved these changes May 22, 2024

View reviewed changes

wvanlint force-pushed the header_provider branch from 92cc59d to 88823a5 Compare May 23, 2024 22:09

wvanlint requested a review from G8XSU May 23, 2024 22:23

G8XSU reviewed May 23, 2024

View reviewed changes

tnull reviewed May 24, 2024

View reviewed changes

wvanlint requested review from G8XSU and tnull July 9, 2024 22:47

tnull approved these changes Jul 12, 2024

View reviewed changes

G8XSU reviewed Jul 17, 2024

View reviewed changes

Introduce header provider trait

b06b37c

wvanlint force-pushed the header_provider branch from 93e5d35 to b06b37c Compare July 17, 2024 21:05

G8XSU approved these changes Jul 17, 2024

View reviewed changes

G8XSU merged commit 59769c9 into lightningdevkit:main Jul 17, 2024

wvanlint deleted the header_provider branch July 17, 2024 21:45

G8XSU mentioned this pull request Aug 23, 2024

Minor version release 0.3 #34

Merged

	/// HTTP headers will be provided by the given `header_provider`.
	///
	/// HTTP headers will be provided by the given `header_provider`.

Introduce header provider trait #31

Introduce header provider trait #31

Uh oh!

Conversation

wvanlint commented May 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU May 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wvanlint commented May 22, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU left a comment

Choose a reason for hiding this comment

Uh oh!

wvanlint commented Jul 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

G8XSU May 15, 2024 •

edited

Loading

tnull May 22, 2024 •

edited

Loading

tnull May 23, 2024 •

edited

Loading

G8XSU May 23, 2024 •

edited

Loading