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

Fix CjManager Notification flow #10527

Merged
merged 14 commits into from Apr 25, 2023

Conversation

adamPetho
Copy link
Collaborator

@adamPetho adamPetho commented Apr 19, 2023

Fixes: #10521

It got messed up in #10441

Previously:
We notified the MusicBox about the PlebStop, but right after that, we overwrote the MusicBox status and the user couldn't override the PlebStop.

With this:
User get to decide if he wants to override the PlebStop or not. Just as before my mistake.

Please test this PR and look out for edge cases.

Szpoti
Szpoti previously approved these changes Apr 19, 2023
Copy link
Collaborator

@Szpoti Szpoti left a comment

Choose a reason for hiding this comment

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

tACK, one concern only

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs Outdated Show resolved Hide resolved
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.

CleanShot.2023-04-19.at.08.14.35.mp4

Before your changes this wasn't happening, user would directly face the PlebStop

@adamPetho
Copy link
Collaborator Author

CleanShot.2023-04-19.at.08.14.35.mp4
Before your changes this wasn't happening, user would directly face the PlebStop

Is this mandatory to fix? I have a solution for it, but it's far from optimal.

@turbolay
Copy link
Collaborator

Unfortunately, I would say yes, it is mandatory.
It introduces a bad flickering + is proof that something fishy is going on.

This logic is extremely tricky (to not say bad) and it would be best to ensure that it is untouched.

yahiheb
yahiheb previously approved these changes Apr 19, 2023
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

tACK, just a small suggestion to fix CI.

WalletWasabi/WabiSabi/Client/CoinJoinManager.cs Outdated Show resolved Hide resolved
@yahiheb yahiheb requested a review from soosr April 19, 2023 17:52
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.

Getting closer, but when I have only unconfirmed funds, I have the message Waiting for coinjoin when I should have Waiting for confirmed funds

@turbolay
Copy link
Collaborator

turbolay commented Apr 20, 2023

I am also stuck on Successful coinjoin! continuing.... if I can't immediately participate in a new round:

  • In the video, it's because all my coins were made private (it should display Hurray!! All your coins are private)
  • In the screenshot, because all my coins are pending (it should display Waiting for confirmed coins)
CleanShot.2023-04-19.at.18.35.28.mp4

CleanShot 2023-04-19 at 18 43 13@2x

@soosr soosr removed their request for review April 20, 2023 07:16
@soosr
Copy link
Collaborator

soosr commented Apr 20, 2023

I can't review this, removing myself.

@Szpoti
Copy link
Collaborator

Szpoti commented Apr 20, 2023

I made a fix for this: Szpoti@3e8bb17
(forgot to remove the comment, trivial)
@adamPetho does it makes sense to you?
@turbolay can you verify it works for you as well?

@adamPetho adamPetho marked this pull request as draft April 20, 2023 09:27
@adamPetho
Copy link
Collaborator Author

I made a fix for this: Szpoti@3e8bb17 (forgot to remove the comment, trivial) @adamPetho does it makes sense to you?

Thanks Szpoti!

I think it makes sense.

@adamPetho
Copy link
Collaborator Author

I am also stuck on Successful coinjoin! continuing.... if I can't immediately participate in a new round

In the video, it's because all my coins were made private (it should display Hurray!! All your coins are private)
In the screenshot, because all my coins are pending (it should display Waiting for confirmed coins)

@turbolay Could you please re-test these scenarios with the latest commit?

@adamPetho adamPetho marked this pull request as ready for review April 20, 2023 11:42
turbolay
turbolay previously approved these changes Apr 20, 2023
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.

tACK

yahiheb
yahiheb previously approved these changes Apr 25, 2023
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

Tested. LGTM

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

tACK

@molnard molnard merged commit b3ca5b5 into zkSNACKs:master Apr 25, 2023
6 of 7 checks passed
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.

PlebStop not working on master
6 participants