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

guardian-ui: fix config verification #268

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Conversation

okjodom
Copy link
Collaborator

@okjodom okjodom commented Nov 7, 2023

At the config verification step after all the guardians have verified other configs, if the leader continued their own setup to completion before the follower guardians, there is a chance the followers would not be able to resume and complete their setup since we relied on their servers being able to continuously poll the leader's server for consensus configuration.

This PR fixes the bug, allowing all guardians to complete federation set-up in any order, provided they have successfully coordinated through config verification.

Steps to validate

  1. Before fix:
  • Follow federation set-up through config verification until you see "All guardians have verified their codes"
  • Reload any guardians setup page to confirm that the verified codes are automatically pre-filled on reload
  • Complete set-up for the leader
  • Reload the page for any other guardian and observe that config verification shows an error. The verification page expects the leader's server to still be in set up and not in consensus running
  1. After fix:
  • Follow federation set-up through config verification until you see "All guardians have verified their codes"
  • Reload any guardians setup page to observe that the verified codes are automatically pre-filled on reload
  • Complete set-up for the leader
  • Reload the page for any other guardian and observe that the verified codes are automatically pre-filled on reload
  • Confirm you can complete set-up for these "follower" guardians

Fixes #255

Kodylow
Kodylow previously approved these changes Nov 7, 2023
README.md Outdated Show resolved Hide resolved
@wbobeirne
Copy link
Collaborator

@okjodom I hit continue with the leader before verifying all configs on all followers, and got stuck on this screen:

CleanShot 2023-11-14 at 13 16 33@2x

Is this a regression from fedimint/fedimint#3547? I was unable to get this to happen in master.

@okjodom
Copy link
Collaborator Author

okjodom commented Nov 14, 2023

@okjodom I hit continue with the leader before verifying all configs on all followers, and got stuck on this screen:

CleanShot 2023-11-14 at 13 16 33@2x

Is this a regression from fedimint/fedimint#3547? I was unable to get this to happen in master.

do you mean current repo master, or upstream master? trying a repro

@wbobeirne
Copy link
Collaborator

Apologies, I meant fedimint/ui@master using the version of devimint pegged in the flake.nix, which was 23ee7cb6e96fce89bb024fbc1fcfccbfb3dc968b at the time of my run.

@okjodom
Copy link
Collaborator Author

okjodom commented Nov 14, 2023

Apologies, I meant fedimint/ui@master using the version of devimint pegged in the flake.nix, which was 23ee7cb6e96fce89bb024fbc1fcfccbfb3dc968b at the time of my run.

Figured I didn't properly update flake lock - now it's taking forever to recompile devimint after bump🤦🏿‍♂️

@okjodom
Copy link
Collaborator Author

okjodom commented Nov 16, 2023

help me close this bug, @wbobeirne , @Kodylow

@Kodylow
Copy link
Member

Kodylow commented Nov 17, 2023

Ran the tests with this yesterday closing leader's tab and refreshing everything, looks good to me.

@okjodom
Copy link
Collaborator Author

okjodom commented Nov 17, 2023

thanks for the review. landing fix

@okjodom okjodom merged commit 8af07e1 into fedimint:master Nov 17, 2023
1 check 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.

Handle being in VerifiedConfigs when leader is in ConsensusRunning
3 participants