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

draft: fix(util): match non-pointer public key types #48

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

TheoTechnicguy
Copy link
Contributor

This PR aims to fix #38. Some pubkeys passed to the function are not pointers, but are still valid keys. This commit fixes this bug by adding the non-pointer types to the switch. Furthermore, more debugging log prints are added.

I am submitting this PR as a draft, as integration tests do not pass (see this comment).

Some `pubkey`s passed are not pointers, but are still valid keys. This
commit fixes this bug by adding the non-pointer types.

fix: 38
@TheoTechnicguy TheoTechnicguy marked this pull request as draft December 17, 2023 10:27
@francoismichel
Copy link
Owner

Thank you for the PR!

It should now work in integration tests if you rebase your branch on main.

What would be nice as well is to add an integration test case with ed25519 similar to the It("Should connect using privkey" test case. I added an ed25519 in the CI scripts accessible in the integration tests through the TESTUSER_ED25519_PRIVKEY environment variable. Would you feel like adding a test case using that key as well ? I can work on that otherwise.

@TheoTechnicguy
Copy link
Contributor Author

TheoTechnicguy commented Dec 17, 2023

I agree that creating a test case for this is a good idea. I'd have to look up how to do that with Ginko, which might take a while. Furthermore, I am working on adding a bit more debug logging (draft PR coming up [edit: #58]), which will help to get better bug reports (also, I already know zerolog, so this is easier for me 😋). Not wanting to discredit the value of testing, the latter one seems more important to me, as it will help with bug reports.

Depending on your judgment, I can hand over none, one or both PRs and/or reprioritize these.

@francoismichel
Copy link
Owner

Do it as you like the most, I can add the testcase on this PR myself as well, it would not take me much time so it is as you prefer. :-)

@TheoTechnicguy
Copy link
Contributor Author

In that case, I'll extend the debug logging ^^.

@francoismichel francoismichel marked this pull request as ready for review December 18, 2023 20:06
@francoismichel francoismichel merged commit 6127e06 into francoismichel:main Dec 18, 2023
7 checks passed
@francoismichel
Copy link
Owner

Merged as 6127e06, thank you !

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.

Unknown signing method: ed25519.PublicKey error?
2 participants