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

Cleanup stacks-signer by adding thiserror and documentation #3883

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Aug 29, 2023

Changes:
Alphabetically order cargo.toml dependencies
Forbids no documentation
Adds a README to be filled in as the CLI and SDK is expanded.
Addresses a bunch of random clippy warnings.
Uses thiserror to simplify boiler plate code for error definitions/formatting.

  • My initial investigations of thiserror show no use of unsafe throughout the code base. Please feel free to audit this crate in case you have concerns about adding it to the stacks-signer lib. If no complaints, I may create some new issues to add it to other sections of the stacks-network repo.

This PR addresses part of #3882

Please add reviewers as you see fit who may have a strong opinion for or against thiserror or the use of a different crate.

@jferrant jferrant added the chore Necessary but less impactful tasks such as cleanup or reorg label Aug 29, 2023
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #3883 (439d4fe) into feat/stacks-signer (0bf6285) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@                 Coverage Diff                 @@
##           feat/stacks-signer    #3883   +/-   ##
===================================================
  Coverage                0.16%    0.16%           
===================================================
  Files                     339      339           
  Lines                  290265   290213   -52     
===================================================
  Hits                      469      469           
+ Misses                 289796   289744   -52     
Files Changed Coverage Δ
libsigner/src/error.rs 0.00% <0.00%> (ø)
libsigner/src/events.rs 0.00% <ø> (ø)
libsigner/src/runloop.rs 0.00% <ø> (ø)
libsigner/src/session.rs 0.00% <ø> (ø)
libsigner/src/tests/http.rs 0.00% <0.00%> (ø)
libsigner/src/tests/mod.rs 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

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

LGTM

@jcnelson
Copy link
Member

Hey @jferrant, this looks like it's otherwise ready to go, but it has a few merge conflicts. Can you resolve these and merge this PR when you get a moment? Thanks!

@jferrant
Copy link
Collaborator Author

Hey @jferrant, this looks like it's otherwise ready to go, but it has a few merge conflicts. Can you resolve these and merge this PR when you get a moment? Thanks!

Oh completely forgot I had this open . yes will do right now.

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant merged commit bbc91d2 into feat/stacks-signer Sep 21, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Necessary but less impactful tasks such as cleanup or reorg
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants