Skip to content

Rename lightning/src/chain/keysinterface.rs to something like signer.rs #2157

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

Closed
ariard opened this issue Apr 6, 2023 · 7 comments · Fixed by #2246
Closed

Rename lightning/src/chain/keysinterface.rs to something like signer.rs #2157

ariard opened this issue Apr 6, 2023 · 7 comments · Fixed by #2246
Labels
good first issue Good for newcomers

Comments

@ariard
Copy link

ariard commented Apr 6, 2023

The file was originally named after the KeysInterface trait which doesn't exist anymore after #1930. This is a bit of confusion for downstream project and our documentation (which sounds to require a cleanup too: https://lightningdevkit.org/key_management/)

@ariard ariard added the good first issue Good for newcomers label Apr 6, 2023
@gantanikhilraj
Copy link

Hello @ariard, I can work on this. Please assign it to me.

@arik-so
Copy link
Contributor

arik-so commented Apr 6, 2023

Hey @ariard, @gantanikhilraj, and @TomNag-CS50, part of an upcoming Taproot PR is actually renaming it to sign, and elevating it to a directory module. We're also moving it outside of chain, and instead make it a sibling directory. Would you like to do that yourselves, or are you fine with waiting for the Taproot PR?

@0xSaksham
Copy link

Hey, this seems like a good issue for me to start. Please point me to further resources if need be

@TheBlueMatt
Copy link
Collaborator

Sounds like something that should happen as a prefactor PR either way, no?

@arik-so
Copy link
Contributor

arik-so commented Apr 10, 2023

Partially. With the upgrade to a directory module for the purpose of having a separate file for Taproot-specific signing operations, it might not make sense outside the context of the Taproot PR, but I'm not that opinionated.

@jkczyz
Copy link
Contributor

jkczyz commented Apr 20, 2023

Hey @ariard, @gantanikhilraj, and @TomNag-CS50, part of an upcoming Taproot PR is actually renaming it to sign, and elevating it to a directory module. We're also moving it outside of chain, and instead make it a sibling directory. Would you like to do that yourselves, or are you fine with waiting for the Taproot PR?

FWIW, I had some proposed naming and further sub-module breakdown in a parallel effort. #2159 (comment)

Thoughts?

@arik-so
Copy link
Contributor

arik-so commented Apr 21, 2023

I think the overarching subject is actually signing, so I'd stick with the sign top-level module name. It makes more sense to put entropy into sign than into key imo. 100% agree on manager. The signer I think should just live in mod.rs. I'd also create submodules for taproot and later maybe ecdsa or legacy or whatnot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants