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

Don't use pub keyword in examples, and don't derive everything but the kitchen sink in examples #1141

Closed
Kixunil opened this issue Jul 28, 2022 · 6 comments
Labels
code quality Makes code easier to understand and less likely to lead to problems good first issue trivial Obvious, easy and quick to review (few lines or doc-only...)

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 28, 2022

pub is useless in binaries and unused derives are not helpful.
Example offender: pub enum SighashError in examples/ecdsa-psbt.rs (disclaimer: didn't actually check if the derives are unused but seems almost certain some are)

@Kixunil Kixunil added documentation good first issue code quality Makes code easier to understand and less likely to lead to problems trivial Obvious, easy and quick to review (few lines or doc-only...) labels Jul 28, 2022
@tcharding
Copy link
Member

As author of ecdsa-psbt.rs I confirm the derives are useless and got there through my mindless cargo cult programming - can definitely be removed.

@sanket1729
Copy link
Member

FTR, I was assuming that the example would be cleaned up #957 which was the main reason that I ACKed it #940 (review).

@lorenzolfm
Copy link
Contributor

Removing pub from psbt_sign::SighashError and from psbt_sign::sign yields visibility erros when running ./contrib/test.sh (see log bellow).

I'm not that experienced in Rust, but from what I've read in the visibility and privacy chapter from the rust book the sign function and SighashError enum should remain pub so both are visible outside the psbt_sign module.

Am I missing something here?

error[E0412]: cannot find type `SighashError` in this scope
   --> examples/ecdsa-psbt.rs:323:17
    |
323 |     PsbtSighash(SighashError),
    |                 ^^^^^^^^^^^^ not found in this scope
    |
help: possible candidate is found in another module, you can import it into scope
    |
31  | use crate::psbt_sign::SighashError;
    |

error[E0412]: cannot find type `SighashError` in this scope
   --> examples/ecdsa-psbt.rs:358:11
    |
358 | impl From<SighashError> for Error {
    |           ^^^^^^^^^^^^ not found in this scope
    |
help: possible candidate is found in another module, you can import it into scope
    |
31  | use crate::psbt_sign::SighashError;
    |

error[E0412]: cannot find type `SighashError` in this scope
   --> examples/ecdsa-psbt.rs:359:16
    |
359 |     fn from(e: SighashError) -> Error { Error::PsbtSighash(e) }
    |                ^^^^^^^^^^^^ not found in this scope
    |
help: possible candidate is found in another module, you can import it into scope
    |
31  | use crate::psbt_sign::SighashError;
    |

error[E0603]: function `sign` is private
   --> examples/ecdsa-psbt.rs:145:20
    |
145 |         psbt_sign::sign(&mut psbt, &sk, 0, secp)?;
    |                    ^^^^

warning: unused import: `self::psbt_sign::*`
  --> examples/ecdsa-psbt.rs:50:5
   |
50 | use self::psbt_sign::*;
   |     ^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

error: aborting due to 4 previous errors

@apoelstra
Copy link
Member

@lorenzolfm thanks for checking on this. For the stuff inside the inline psbt_sign module, we should change the pubs to pub(crate) or even pub(super) to explicitly indicate that we only intend to give visibility to the surrounding module.

I'm not sure in detail what @sanket1729 means by "cleaning up" this example but it may be that we can entirely delete the module after #957 merges and then we can avoid this altogether :).

@tcharding
Copy link
Member

Yes correct, the module is temporary while we work on the signing API (#957). I like pub(super) if that makes the compiler happy.

@tcharding
Copy link
Member

With the removal of the signing code from ecdsa-psbt this issue no longer exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Makes code easier to understand and less likely to lead to problems good first issue trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Development

No branches or pull requests

5 participants