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

Build code from docs #407

Merged
merged 7 commits into from
May 17, 2024
Merged

Build code from docs #407

merged 7 commits into from
May 17, 2024

Conversation

Randy808
Copy link
Collaborator

@Randy808 Randy808 commented Apr 15, 2024

The docs for this branch use the latest version of the code and this shouldn't be merged yet until the below tasks have been completed.

  • Distribute latest release on crates.io and pypi
  • Remove extension traits that have methods we want to merge into gl-client
    - [ ] Update docs for creds page
  • Add python examples
  • Add jobs to ensure both Rust and Python files are being validated (maybe we should validate Python files with mypy?)

Copy link
Collaborator

@ErikDeSmedt ErikDeSmedt left a comment

Choose a reason for hiding this comment

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

Question:
Why is there a ca.crt-file in python/getting-started but not in rust/getting-started?

@Randy808
Copy link
Collaborator Author

The standard way to initialize credentials is by using the associated function with which takes in the cert and key used for auth, and the CA cert. This differs from the way the TlsConfig was able to load the CA cert, where something closer to a builder pattern could be used to call TlsConfig::new().unwrap().identity(cert, key) and the default CA cert would get loaded without being specified (since it was the default CA value in new).

Because specifying a CA cert every time can be tedious for the average consumer who only wants to talk to the official Greenlight server, and because we don't have instructions in our docs on manually loading up the Greenlight CA cert, I thought it would be beneficial to add a convenience functions that take care of this for the developer. The convenience method I used was with_identity, and I defined these on an extension trait for the Credentials called CredentialExt. I make my convenience method by initializing the default impl for each credential, and manually changing the values of the fields related to the identity.

In Python these fields aren't exposed so this approach doesn't work. Instead, I just put the Greenlight CA cert in the same folder as the main.py python file so my convenience method nobody_with_identity can call the with mentioned above and pass in the cert in the directory as the CA cert (so the developer doesn't have to worry about it).

I have another PR to make it easier to initialize a Credential without explicitly specifying the cert but I put custom functions in the docs as placeholders. It gets a little tricky with how it's done because of the UnifiedCredentials used to simplify the Python API but I think it's starting to get into a state I'm happy with.

@ErikDeSmedt
Copy link
Collaborator

Could we bundle the ca.cert as a part of the gl-client-package?
You should be able to extract it easily using the __file__-variable which contains the path to the current file.

Copy link
Collaborator

@nepet nepet left a comment

Choose a reason for hiding this comment

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

Very nice work @Randy808!

I just have some minor stuff. I'll add some thoughts about the credentials extensions to #412 directly

examples/python/getting-started/main.py Outdated Show resolved Hide resolved
examples/rust/getting-started/src/main.rs Outdated Show resolved Hide resolved
docs/src/getting-started/register.md Show resolved Hide resolved
examples/rust/getting-started/src/main.rs Show resolved Hide resolved
docs/src/getting-started/recover.md Show resolved Hide resolved
@cdecker
Copy link
Collaborator

cdecker commented May 14, 2024

@Randy808 how is this PR looking, can we merge it soon even if not quite perfect, we can always fix up later, as long as we don't forget about it.

@Randy808 Randy808 force-pushed the build-code-from-docs branch 2 times, most recently from 0f2f2cf to 36f7e60 Compare May 15, 2024 19:50
@Randy808
Copy link
Collaborator Author

I didn't update the credentials doc but I can make that a separate issue. The only other thing on the checklist is publishing new versions of glclient

@Randy808 Randy808 marked this pull request as ready for review May 17, 2024 11:18
nepet
nepet previously approved these changes May 17, 2024
Copy link
Collaborator

@nepet nepet left a comment

Choose a reason for hiding this comment

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

ack 4ffebe6

@nepet nepet enabled auto-merge (rebase) May 17, 2024 12:29
@nepet nepet merged commit d30b571 into Blockstream:main May 17, 2024
19 checks passed
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.

4 participants