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.

Sounds good to me !
As we don't have any way of handling more granular error types.

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.

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.

G8XSU · 2024-05-23T23:16:44Z

src/headers/mod.rs

+	/// Any returned errors should be of the `VssError::AuthError` variant,
+	/// or will be mapped to it.


Suggested change

/// Any returned errors should be of the `VssError::AuthError` variant,

/// or will be mapped to it.

/// Any returned errors must be of the `VssError::AuthError` variant.

Also, lets link to it, i.e.

Suggested change

/// Any returned errors should be of the `VssError::AuthError` variant,

/// or will be mapped to it.

/// Any returned errors should be of the [`VssError::AuthError`] variant,

/// or will be mapped to it.

G8XSU · 2024-05-23T23:23:32Z

src/client.rs

+			.map_err(|e| match e {
+				e @ VssError::AuthError(_) => e,
+				e => VssError::AuthError(e.to_string()),
+			})?;


Suggested change

.map_err(|e| match e {

e @ VssError::AuthError(_) => e,

e => VssError::AuthError(e.to_string()),

})?;

.map_err(|e| match e {

VssError::AuthError(_) => e,

_ => VssError::AuthError(e.to_string()),

})?;

nit: @ Binding seems unnecessary here.

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

@@ -34,7 +39,19 @@ impl<R: RetryPolicy<E = VssError>> VssClient<R> {

 	/// 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.

tnull · 2024-05-24T06:39:58Z

src/headers/mod.rs

+	/// Any returned errors should be of the `VssError::AuthError` variant,
+	/// or will be mapped to it.


Also, lets link to it, i.e.

Suggested change

/// Any returned errors should be of the `VssError::AuthError` variant,

/// or will be mapped to it.

/// Any returned errors should be of the [`VssError::AuthError`] variant,

/// or will be mapped to it.

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.

wvanlint mentioned this pull request May 15, 2024

JWT authorization header based on LNURL Auth #26

Open

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 tnull and G8XSU May 22, 2024 23:06

G8XSU approved these changes May 22, 2024

View reviewed changes

Introduce header provider trait

88823a5

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce header provider trait #31

Introduce header provider trait #31

wvanlint commented May 15, 2024

G8XSU May 15, 2024

wvanlint May 16, 2024

G8XSU May 15, 2024 •

edited

wvanlint May 16, 2024

G8XSU May 16, 2024

tnull May 22, 2024 •

edited

wvanlint May 22, 2024

G8XSU May 22, 2024

tnull May 23, 2024 •

edited

G8XSU May 23, 2024 •

edited

wvanlint May 23, 2024

tnull May 24, 2024

G8XSU May 15, 2024

wvanlint May 16, 2024

wvanlint commented May 22, 2024

G8XSU May 23, 2024

tnull May 24, 2024

G8XSU May 23, 2024

G8XSU May 23, 2024

tnull May 24, 2024

tnull May 24, 2024

tnull May 24, 2024

		/// Any returned errors should be of the `VssError::AuthError` variant,
		/// or will be mapped to it.

	/// Any returned errors should be of the `VssError::AuthError` variant,
	/// or will be mapped to it.
	/// Any returned errors must be of the `VssError::AuthError` variant.

		@@ -34,7 +39,19 @@ impl<R: RetryPolicy<E = VssError>> VssClient<R> {

		/// 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 {

Introduce header provider trait #31

Are you sure you want to change the base?

Introduce header provider trait #31

Conversation

wvanlint commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G8XSU May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull May 23, 2024 • edited

Choose a reason for hiding this comment

G8XSU May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvanlint commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G8XSU May 15, 2024 •

edited

tnull May 22, 2024 •

edited

tnull May 23, 2024 •

edited

G8XSU May 23, 2024 •

edited