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

[feature] return a proper errors instead of debug asserts #1165

Merged
merged 8 commits into from
Jan 16, 2025

Conversation

MCJOHN974
Copy link
Collaborator

@MCJOHN974 MCJOHN974 commented Dec 18, 2024

Description

Closes: #1164

Changes

Testing Information

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

signer/src/error.rs Outdated Show resolved Hide resolved
@MCJOHN974
Copy link
Collaborator Author

I will replace other debug_assert_* in another commits, WIP

@djordon
Copy link
Collaborator

djordon commented Dec 20, 2024

I will replace other debug_assert_* in another commits, WIP

Before replacing them, try to figure out whether we should just remove them instead. That's a little harder to do, but give it a shot.

@MCJOHN974
Copy link
Collaborator Author

With all this changes we will have only one debug_assert in the codebase:

debug_assert_eq!(self.deposit_sighashes.len(), self.reports.deposits.len());

Since the function where it is located do not return a Result but just a Vec we are okish to just panic inside this function which debug_assert will do.

@MCJOHN974 MCJOHN974 marked this pull request as ready for review January 6, 2025 14:20
@MCJOHN974 MCJOHN974 requested a review from djordon January 6, 2025 14:20
signer/src/stacks/api.rs Outdated Show resolved Hide resolved
signer/src/error.rs Outdated Show resolved Hide resolved
signer/src/error.rs Outdated Show resolved Hide resolved
signer/src/error.rs Outdated Show resolved Hide resolved
signer/src/error.rs Outdated Show resolved Hide resolved
@MCJOHN974 MCJOHN974 requested a review from djordon January 8, 2025 10:22
@MCJOHN974
Copy link
Collaborator Author

@djordon thanks for your comments! I think last commit fixes issues you mentioned

Copy link
Collaborator

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Looks good, I just had one rewording comment.

signer/src/error.rs Outdated Show resolved Hide resolved
Co-authored-by: Daniel Jordon <djordon@users.noreply.github.com>
@MCJOHN974 MCJOHN974 merged commit d80f097 into main Jan 16, 2025
4 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.

[Feature]: Return a proper errors instead of a debug_asserts
2 participants