Skip to content

changed keysinterface.rs to signer.rs #2159

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

Conversation

TomNag-CS50
Copy link

changed keysinterface.rs to to signer.rs

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Dunno if signer is right given it also includes EntropySource, but I think its better than keysinterface. Maybe let's do something like "crypto"? Dunno if that's too generic.

While you're at it can you move it out of the chain module and into the top-level lightning crate/lib?

../../lightning/src/util/time.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you made this no longer a symlink, which broke build?

Copy link
Author

Choose a reason for hiding this comment

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

ill check again, but running it on main branch unchanged produced the same error.

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 your git/OS/filesystem bungled the symlink and somehow replaced it with a file, which is why it failed locally and failed once checked in.

@jkczyz
Copy link
Contributor

jkczyz commented Apr 6, 2023

Dunno if signer is right given it also includes EntropySource, but I think its better than keysinterface. Maybe let's do something like "crypto"? Dunno if that's too generic.

While you're at it can you move it out of the chain module and into the top-level lightning crate/lib?

How about a keys top-level module with signer, entropy, and manager submodules?

@@ -411,7 +411,8 @@ enum OnchainEvent {
/// If the funding spend transaction was a known remote commitment transaction, we track
/// the output index and amount of the counterparty's `to_self` output here.
///
/// This allows us to generate a [`Balance::CounterpartyRevokedOutputClaimable`] for the
/// This allows us to generate a [`Balance::CounterpartyRevokedOutputClaimable`] for
/// thechaannmo
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: /// thechaannmo, also should use spaces not tabs (though the full line will be deleted so it doesn't matter much here).

@arik-so
Copy link
Contributor

arik-so commented Apr 20, 2023

Dunno if signer is right given it also includes EntropySource, but I think its better than keysinterface. Maybe let's do something like "crypto"? Dunno if that's too generic.
While you're at it can you move it out of the chain module and into the top-level lightning crate/lib?

How about a keys top-level module with signer, entropy, and manager submodules?

Ugh, we have separate parallel threads: #2157 (comment)

@jkczyz
Copy link
Contributor

jkczyz commented Apr 20, 2023

Ugh, we have separate parallel threads: #2157 (comment)

Seems like progress has stall here and you may do this as part of some other work @arik-so?

@TheBlueMatt
Copy link
Collaborator

@TomNag-CS50 any interest in redoing this with the suggestions in the thread at #2157?

@TheBlueMatt
Copy link
Collaborator

Superseded by #2246. More splitting of the sign module would be welcome in a followup PR, if you find the time, @TomNag-CS50.

@TheBlueMatt TheBlueMatt closed this May 3, 2023
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.

5 participants