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

#2761 Followups #3004

Merged
merged 6 commits into from
Apr 25, 2024
Merged

Conversation

TheBlueMatt
Copy link
Collaborator

Followups from #2761 (review)

@TheBlueMatt TheBlueMatt added this to the 0.0.123 milestone Apr 19, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (2c0fcf2) to head (6ab91cb).
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3004      +/-   ##
==========================================
- Coverage   89.13%   89.11%   -0.03%     
==========================================
  Files         118      118              
  Lines       97492    97492              
  Branches    97492    97492              
==========================================
- Hits        86903    86883      -20     
- Misses       8349     8367      +18     
- Partials     2240     2242       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -38,18 +38,18 @@ macro_rules! basepoint_impl {
self.0
}

/// Derives a per-commitment-transaction (eg an htlc key or delayed_payment key) private key addition tweak
/// from a basepoint and a per_commitment_point:
/// Derives the "tweak" used in calculate [`Self::from_basepoint`].
Copy link
Contributor

Choose a reason for hiding this comment

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

The compiler doesn't like this reference to Self within the macro :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well that's cause its actually on a different type...

We assume that tweaks are the output of a SHA-256 hash function
(and thus that failing to create a private key from the has
negligible probability) in `add_public_key_tweak` and elsewhere.

Thus, we really shouldn't be taking byte arrays in the public API
but rather `Sha256` objects, and communicating in the docs for
`add_public_key_tweak` that we can panic if its not the output of
a hash function, both of which we do here.
Specifically `RevocationBasepoint` has a different derivation, so
shouldn't have a `derive_add_tweak` at all. We also use this
opportunity to link to the `from_basepoint` function in the
`derive_add_tweak` docs.
Since its already formatted and there's not much active work going
on in it.
The +rustversion call semantics are specific to rustup and do not
work with standard rust toolchains. However, because rustfmt
formatting differs slightly between stable and our 1.63 target, we
need to keep the +1.63.0 for rustup users.

Thus, here, we check for the presence of a `rustup` command and
pass the "+1.63.0" argument if we find one.
@TheBlueMatt
Copy link
Collaborator Author

Fixed the docs and also noted that we shouldn't be exposing derive_add_tweak for the revocation key at all - the derivation is different there. Also set channel_keys to be rustfmt'd, because it already passes and after this PR probably no one is working on it.

@TheBlueMatt
Copy link
Collaborator Author

Changes are just (a) rustfmt, (b) a doc comment and one method removal, (c) one parameter type change, so gonna go ahead and merge this.

@TheBlueMatt TheBlueMatt merged commit eebab40 into lightningdevkit:main Apr 25, 2024
16 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.

None yet

3 participants