Skip to content

feat: initial gnap and http signatures implementation#10

Merged
Tymmmy merged 43 commits intointerledger:mainfrom
elijah0kello:ft/gnap-utils
Oct 2, 2025
Merged

feat: initial gnap and http signatures implementation#10
Tymmmy merged 43 commits intointerledger:mainfrom
elijah0kello:ft/gnap-utils

Conversation

@elijah0kello
Copy link
Copy Markdown
Contributor

@elijah0kello elijah0kello commented Jun 17, 2025

Changes in this PR

  • added classes for key management
  • added http signature header support for requests to auth server and resource server
  • added unit tests
  • added github actions file to run tests

Things to note

  • integration tests are still failing since when I was making requests to https://auth.interledger-test.dev, I didn't have any public keys registered.
  • I therefore need some help understanding how to upload public keys to the AS.

@Tymmmy @johngian please review

Authors

Run unit tests

poetry run pytest -s test/unit

@elijah0kello
Copy link
Copy Markdown
Contributor Author

@Tymmmy I have added some usage documentation in the README.md. Still adding more docs though.

The current docs should give an idea of how I envisioned the client to be used.

@elijah0kello
Copy link
Copy Markdown
Contributor Author

The integration test for requesting a grant is now passing.

to run integration tests.

poetry run pytest -s tests/integration/test_grants.py

I registered a wallet , created keys and loaded it in privkey.pem.example for testing

Copy link
Copy Markdown
Member

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

Looking good!
Did a quick first pass, will try setting up locally for next round of review.

Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread README.md
Comment thread src/open_payments_sdk/api/auth.py Outdated
Comment thread src/open_payments_sdk/gnap_utils/security.py Outdated
Comment thread src/open_payments_sdk/gnap_utils/keys.py Outdated
Comment thread src/open_payments_sdk/http.py
Comment thread src/open_payments_sdk/http.py Outdated
@elijah0kello
Copy link
Copy Markdown
Contributor Author

thanks @sidvishnoi for these comments. I will address them ASAP.

@johngian
Copy link
Copy Markdown
Contributor

Some overall comments about this PR:

  • I think 33 commits are not very manageable/readable to review. I suggest the commits to be more atomic.
  • I am not sure what's the policy for 3rd party libraries but I would advise against building on top of non established authentication related libraries

@elijah0kello
Copy link
Copy Markdown
Contributor Author

thanks @johngian for these comments.

  • I am happy to take you through the codebase if it is overwhelming. Also it is just a lot of commits but the actual code changes are not so many.
  • In regard to the third party lib, I read through the code base for the http signatures lib and also made a PR to fix something that was wrong. That being said if there is a policy, let's hear it and I'll address that specific issue with a custom implementation of http signatures.

@johngian
Copy link
Copy Markdown
Contributor

johngian commented Jul 14, 2025

@elijah0kello its not about having an overwhelming codebase. There is a purpose and value on having atomic commits. The PR at its current state (from a quick look) has 4+ commits that are about running unit tests and 3-4 commits only about updating docs, on top of that there is a bunch of merge commits from other repositories which don't bring much value in the context of the commit history. Also there is a commit that fixes a typo of a change introduced in this changeset.

Regarding the http signatures lib i would defer to the folks from the organization to decide but if it was my decision i would rather have an implementation that is audited/vetted by the org or built internally just for the purpose of ILF projects.

Comment thread README.md
Install it in your project

```bash
pip install </path/to/>open-payments-python-sdk/dist/open_payments_sdk-0.1.0-py3-none-any.whl
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We do have a poetry based setup, there is no need to point to a pinned version of the wheel file.

Comment thread README.md
To use this sdk, you will first need to install it in your project. Currently you will need to build from source but once it is hosted on pypi you will be able to install it with pip

```bash
python3 -m pip install open-payments-python-sdk #currently not setup
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you are running poetry install why do you need to explicitly install the module?

@koekiebox
Copy link
Copy Markdown

@elijah0kello its not about having an overwhelming codebase. There is a purpose and value on having atomic commits. The PR at its current state (from a quick look) has 4+ commits that are about running unit tests and 3-4 commits only about updating docs, on top of that there is a bunch of merge commits from other repositories which don't bring much value in the context of the commit history. Also there is a commit that fixes a typo of a change introduced in this changeset.

Regarding the http signatures lib i would defer to the folks from the organization to decide but if it was my decision i would rather have an implementation that is audited/vetted by the org or built internally just for the purpose of ILF projects.

I am happy, as long as we introduce CVE scanning to ensure the libraries are safe to use. I have left other comments on the PR. 🙇

Copy link
Copy Markdown

@koekiebox koekiebox left a comment

Choose a reason for hiding this comment

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

Left a couple of comments, mostly regarding pip-audit.

Comment thread .gitignore Outdated
Comment thread pyproject.toml
Comment thread .github/workflows/test.yml Outdated
@elijah0kello
Copy link
Copy Markdown
Contributor Author

thanks @koekiebox for comments.

I will address as advised. Especially the dependency auditing.

@Tymmmy
Copy link
Copy Markdown
Contributor

Tymmmy commented Oct 1, 2025

@elijah0kello we will probably merge this PR soon, but please for next time try to do smaller PRs, that involve less file changes. I agree with some of the comments above that this was maybe unnecesarry complex.

Copy link
Copy Markdown
Contributor

@Tymmmy Tymmmy left a comment

Choose a reason for hiding this comment

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

Approved wuth the note that this PR would need more work, but in our case it makes more sense to merge this, and proceed to refactor and continue work with this merged.

@Tymmmy Tymmmy merged commit b855fab into interledger:main Oct 2, 2025
@elijah0kello
Copy link
Copy Markdown
Contributor Author

Thanks @Tymmmy

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.

6 participants