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

KeyManager: Introduce GetBestHeight(SyncType) #12212

Merged

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Jan 8, 2024

Not sure if it's a good idea or not. WDYT @turbolay?

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

I see no harm in this.
I designed that way while implementing TurboSync because my rule of thumb for living a happy life is to never touch existing behavior in the KeyManager

It's however true that the API simply makes more sense that way.
I believe that in the same PR we could remove SetBestHeights, and apply the same change and remove SetTurboSyncHeight in favour of SetHeight(syncType).

@kiminuo
Copy link
Collaborator Author

kiminuo commented Jan 16, 2024

What I can imagine is to modify KeyManager in the following way:

-public void SetBestHeight(Height height, bool toFile = true)
-public void SetBestTurboSyncHeight(Height height, bool toFile = true)
-public Height GetBestHeight()

and one would be responsible to call KeyManager.Method(SyncType, ...). WDYT?

I find that better because it forces people to think about the sync type (as it is important)

@kiminuo kiminuo changed the title [PoC] KeyManager: Introduce GetBestHeight(SyncType) KeyManager: Introduce GetBestHeight(SyncType) Jan 16, 2024
@kiminuo kiminuo marked this pull request as ready for review January 16, 2024 12:39
adamPetho
adamPetho previously approved these changes Jan 16, 2024
Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

tACK

@turbolay
Copy link
Collaborator

and one would be responsible to call KeyManager.Method(SyncType, ...). WDYT?
I find that better because it forces people to think about the sync type (as it is important)

I think it's better as well.

@kiminuo kiminuo merged commit a62c83c into WalletWasabi:master Jan 17, 2024
7 checks passed
@kiminuo kiminuo deleted the feature/2024-01-08-keymanager-refactor branch January 17, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants