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

Prevent "no multisig" from being displayed before we checked #390

Merged
merged 10 commits into from
Oct 11, 2023

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Oct 6, 2023

closes #381

The flow of things was as follow

  • connect to a node
  • check whether this is an ethereum network we're connecting to
  • --> here we showed the "no multisig" before even checking
  • depending on the type of network, we request specific accounts (of type substrate or ethereum)
  • query the indexer for known multisigs

There are other flashing things like the "loading multisig" but it's a little more tricky to fix.

  • fixed some nits like stray console.log
  • prevent walletconnect from spamming the console

Submission checklist:

Layout

  • Change inspected in the desktop web ui
  • Change inspected in the mobile web ui

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 9, 2023

Deployment is failing because of Cloudflare issues. The reviewing of this PR shouldn't be impacted though.

@Tbaut Tbaut added the Status: Do not merge Added to PRs that are not allowed to be merged. label Oct 9, 2023
@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 9, 2023

Found a bug, let's not merge.

@github-actions github-actions bot requested a deployment to multix (Preview) October 9, 2023 15:28 Abandoned
@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 9, 2023

The bug was visible when you had a watched account with no multisig, but no extension connection. The spinner would show endlessly.

I changed a little my strategy. Making sure the first "Mutlxi is an interface.." would only show when we're sure we've init all the localstorage related elements (connection to the extension and watched accounts).
The "no multisig" is still displayed, due to the way our state is handled and loosely coupled. But we're now talking about a couple ms, which is less than the blink of an eye, as I think a big refactor to prevent this isn't worth it rn.

@github-actions github-actions bot requested a deployment to multix (Preview) October 9, 2023 15:40 Abandoned
@Tbaut Tbaut requested a review from Lykhoyda October 9, 2023 15:44
@@ -55,7 +55,7 @@ Cypress.Commands.add('initExtension', (accounts: InjectedAccountWitMnemonic[]) =
})

Cypress.Commands.add('getAuthRequests', () => {
return cy.wrap(extension.getAuthRequests())
return cy.wait(500).then(() => cy.wrap(extension.getAuthRequests()))
Copy link
Collaborator

@Lykhoyda Lykhoyda Oct 10, 2023

Choose a reason for hiding this comment

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

We can use waitUntil especially when we show the loader. It's hard to guess how much time you really need, so the test might be unstable.
An example of usage

 cy.waitUntil(function() {
  return cy.get('loader').should('not.exist');
 })

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a library for that method https://www.npmjs.com/package/cypress-wait-until

It's a missing method in the cypress, but it's inspired by "Testing library" (https://testing-library.com/docs/guide-disappearance/#waiting-for-disappearance).

Copy link
Collaborator Author

@Tbaut Tbaut Oct 10, 2023

Choose a reason for hiding this comment

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

I really dislike waits, and I'll fight them all as much as I can. But in this case, I don't think we have any other solution, because the actual firing of the request is itself delayed. the loader appears right away (after this PR) but the firing of the request for accounts is done 500ms after, because of a bug, see the comment above the following code:

// race condition see https://github.com/polkadot-js/extension/issues/938
setTimeout(() => {
getaccountList(chainInfo.isEthereum)
}, 500)

Copy link
Collaborator Author

@Tbaut Tbaut Oct 10, 2023

Choose a reason for hiding this comment

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

I'll check if waitUntil can help here, at least to make things more elegant on the surface. It will have to wait for 500ms at least.

)
}

if (isLoading) {
if (isLoadingMultisigs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about to move the loading and error handling in the separate file:

import React from 'react';
import { CircularProgress } from '@mui/material';
import { LoaderBoxStyled, MessageWrapper, ErrorMessageStyled } from './YourStyledComponents'; // Update with your actual imports

const useLoadingAndError = ({ api, isAccountLoading, isLoadingMultisigs, multisigQueryError, multiProxyList, showNewMultisigAlert, selectedNetworkInfo }) => {
  if (!api) {
    return (
      <LoaderBoxStyled data-cy="loader-rpc-connection">
        <CircularProgress />
        {`Connecting to the node at ${selectedNetworkInfo?.rpcUrl}`}
      </LoaderBoxStyled>
    );
  }

  if (isAccountLoading) {
    return (
      <LoaderBoxStyled data-cy="loader-accounts-connection">
        <CircularProgress />
        Loading accounts...
      </LoaderBoxStyled>
    );
  }

  if (isLoadingMultisigs) {
    return (
      <MessageWrapper>
        <CircularProgress />
        <div>Loading your multisigs...</div>
      </MessageWrapper>
    );
  }

  if (multisigQueryError) {
    return (
      <MessageWrapper>
        <ErrorMessageStyled>
          <ErrorOutlineIcon size={64} />
          <div>An error occurred.</div>
        </ErrorMessageStyled>
      </MessageWrapper>
    );
  }

  if (multiProxyList.length === 0) {
    return (
      <MessageWrapper>
        {showNewMultisigAlert ? <SuccessCreation /> : <ConnectOrWatch />}
      </MessageWrapper>
    );
  }

  return null; 
};

export default useLoadingAndError;

Home page

  const loadingAndErrorComponent = useLoadingAndError({
    api,
    isAccountLoading,
    isLoadingMultisigs,
    multisigQueryError,
    multiProxyList,
    showNewMultisigAlert,
    selectedNetworkInfo,
  });

  if (loadingAndErrorComponent) {
    return loadingAndErrorComponent;
  }

Copy link
Collaborator Author

@Tbaut Tbaut Oct 10, 2023

Choose a reason for hiding this comment

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

Sure, that'd help make the Home easier to read, I may do it in a normal component though, rather than a hook.
edit: a hook allows to remove a lot of code from Home, so doing this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pushed an update separating DisplayError and DisplayLoader, and keeping the rest as is for now.

Copy link
Collaborator

@Lykhoyda Lykhoyda left a comment

Choose a reason for hiding this comment

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

Please check the refactoring suggestion. Let me know if you want to work on it in the separate issue

@Tbaut
Copy link
Collaborator Author

Tbaut commented Oct 10, 2023

All right, waitUntil is much nicer. I want to keep it out of the cypress commands though so as not to request more deps from our users when the plugin is deployed out of Multix

@Tbaut Tbaut requested a review from Lykhoyda October 10, 2023 13:49
Copy link
Collaborator

@Lykhoyda Lykhoyda left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for the updates

@Tbaut Tbaut merged commit f3040eb into main Oct 11, 2023
6 checks passed
@Tbaut Tbaut deleted the tbaut-no-multisig-flashing branch October 11, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Do not merge Added to PRs that are not allowed to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Message "no multisig found for your accounts" showed briefly on refresh although there are multisigs
3 participants