Skip to content
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

Enable using VssStore with VssHeaderProvider. #369

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

G8XSU
Copy link
Contributor

@G8XSU G8XSU commented Oct 10, 2024

  • Enable using VssStore with VssHeaderProvider.
  • Default to LnUrlJWT auth for using VSS.

pub fn build_with_vss_store(&self, url: String, store_id: String) -> Result<Node, BuildError> {
use bitcoin::key::Secp256k1;

pub fn build_with_vss_store_and_header_provider(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should expose three variants on the builder: a) with header based auth (which also would allow to leave them unset allowing unauthenticated use, e.g., for testing) b) Jwt-based auth c) custom authentication via dyn VssHeaderProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I don't mind adding another method for fixed headers. It should be helpful for tests.

})?;

let lnurl_auth_jwt_provider =
LnurlAuthToJwtProvider::new(lnurl_auth_xprv, lnurl_auth_server_url, fixed_headers)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused: the fixed HTTP header variant is unrelated to the JWT-based authentication, no? So shouldn't we here use the FixedHeaders to parametrize the client directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to allow additional custom headers in case vss provider is using JWT-based auth.
Not entirely sure but I think one of client might want to use it.

Copy link
Collaborator

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

CI is failing on the formatting step.

@G8XSU
Copy link
Contributor Author

G8XSU commented Oct 15, 2024

@tnull
Copy link
Collaborator

tnull commented Oct 15, 2024

Pushed 2 more commits but its not showing up since github is still processing them since 3 hrs: https://github.blog/changelog/2023-09-26-more-details-provided-when-a-pull-request-is-merged-indirectly-or-is-still-processing-updates/#pushed-commits-are-still-being-processed.

Hmm, could you force-push once more? Can't review commits that aren't there 😁

@tnull
Copy link
Collaborator

tnull commented Oct 15, 2024

Also needs a rebase to account for the yanked 0.0.124.

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.

2 participants