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

Allow database begin_batch functions to fail #850

Closed
wants to merge 1 commit into from

Conversation

Mic92
Copy link

@Mic92 Mic92 commented Feb 1, 2023

When dealing with local or remote databases than beginning a transaction can fail. Not allowing implementations to return a result and resorting to unwrap() is a non-starter for reliable production code as this will skip Rust's own drop handler in many places and crashes the application.

I have not updated documentation / changelog yet, but would appreciate feedback on the change. I have not run yet any tests.

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits (with ssh)
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@Mic92 Mic92 force-pushed the begin-batch-result branch 3 times, most recently from c5b995c to 77123dd Compare February 1, 2023 14:44
@notmandatory
Copy link
Member

notmandatory commented Feb 1, 2023

Concept ACK.

The blockchain sync and database code is completely changing with the bdk_core work that is underway. But until that code is merged into BDK proper I think it makes sense to remove any .unwrap()s we can.

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Concept + utACK 77123dd

I will go through the code again for a deeper review. And we should probably handle such other unwraps elsewhere too if remains..

@notmandatory
Copy link
Member

@Mic92 we had an issue with our CI, please rebase.

When dealing with local or remote databases than beginning a transaction
can fail. Not allowing implementations to return a result and resorting
to `unwrap()` is a non-starter for reliable production code as this will
skip Rust's own drop handler in many places and crashes the application.

Signed-off-by: Jörg Thalheim <joerg@thalheim.io>
@Mic92 Mic92 force-pushed the begin-batch-result branch from 77123dd to 3b8555c Compare February 21, 2023 14:18
@Mic92
Copy link
Author

Mic92 commented Feb 21, 2023

rebased. I made good experience with using bors as a merge bot for things like rebasing and testing against latest HEAD. Github is now adding something similar called merge queue.

@notmandatory
Copy link
Member

Thanks for all your work on this but in preparation for our new bdk 1.0.0 work we've gotten to the point where I'd like to hold off on merging any new changes and only merge critical bug fixes to the release/0.27 branch.

The way persistence is done after the bdk_core_staging stuff is merged will be very different so this PR shouldn't apply. But please take a look after #793 is merged, I'd appreciate your review around the new persistence code.

@notmandatory notmandatory removed this from the Release 0.27.2 Feature Freeze milestone Mar 3, 2023
@danielabrozzoni
Copy link
Member

Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants