Skip to content

Utxo crate cleanup #353

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

Merged
merged 16 commits into from
Aug 16, 2022
Merged

Utxo crate cleanup #353

merged 16 commits into from
Aug 16, 2022

Conversation

azarovh
Copy link
Contributor

@azarovh azarovh commented Aug 10, 2022

Note: this PR is based on #333 which is not merged yet.

Decided to clean up a crate a bit before introducing any new changes. I think these changes will increase readability and make the code cleaner.

  • Changed the layout of the crate
  • Reworked pub declarations and use directives
  • removed redundant dead_code and dead_imports with useless todos.

@TheQuantumPhysicist
Copy link
Contributor

This looks mostly fine. Just a few things have to be thought through, especially the flags vs booleans thing. Booleans are a MUCH better solution than flags, but there's certainly solutions better than booleans, which we postponed for priority reasons. When I first read that you changed them to flags before looking into the PR, I was happy, because I thought we created some rusty solution that's type-safe... but it seems like we're back at C/C++ interface stuff with dangerously ambiguous non-type-safe solutions. We can certainly do better! If we don't want to do this now, then we better go back to booleans. At least booleans won't accept the value 0x56 at compile-time.

@iljakuklic
Copy link
Contributor

iljakuklic commented Aug 15, 2022

This looks mostly fine. Just a few things have to be thought through, especially the flags vs booleans thing. Booleans are a MUCH better solution than flags, but there's certainly solutions better than booleans, which we postponed for priority reasons. When I first read that you changed them to flags before looking into the PR, I was happy, because I thought we created some rusty solution that's type-safe... but it seems like we're back at C/C++ interface stuff with dangerously ambiguous non-type-safe solutions. We can certainly do better! If we don't want to do this now, then we better go back to booleans. At least booleans won't accept the value 0x56 at compile-time.

I think a modest improvement over bools is a custom enum for each of these, like:

enum IsDirty { Yes, No }

Similarly for other bool-y things. These are all still basically bools but at least it's nicely explicit and it does not allow you to pass one in place of another without an explicit conversion of some sort.

Base automatically changed from fix/utxo-simulation to master August 15, 2022 07:53
Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

A couple of minor comments. Please address @TheQuantumPhysicist's comment on UtxosDBInMemoryImpl initialisation. Looks good otherwise

@TheQuantumPhysicist
Copy link
Contributor

This looks mostly fine. Just a few things have to be thought through, especially the flags vs booleans thing. Booleans are a MUCH better solution than flags, but there's certainly solutions better than booleans, which we postponed for priority reasons. When I first read that you changed them to flags before looking into the PR, I was happy, because I thought we created some rusty solution that's type-safe... but it seems like we're back at C/C++ interface stuff with dangerously ambiguous non-type-safe solutions. We can certainly do better! If we don't want to do this now, then we better go back to booleans. At least booleans won't accept the value 0x56 at compile-time.

I think a modest improvement over bools is a custom enum for each of these, like:

enum IsDirty { Yes, No }

Similarly for other bool-y things. These are all still basically bools but at least it's nicely explicit and it does not allow you to pass one in place of another without an explicit conversion of some sort.

Like I mentioned in the daily, a type-safe flags, where we wrap u8 with a struct that has associated constant values is also OK from my end. So either that enum from Lukas or this type-safe flags thing. Whichever is fine for me.

@azarovh azarovh merged commit 09ffd54 into master Aug 16, 2022
@azarovh azarovh deleted the utxo-cleanup branch August 16, 2022 09:29
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.

4 participants