skalenetwork / filestorage-ui

filestorage-ui.vercel.app
GNU Lesser General Public License v3.0
2 stars 3 forks source link

Uploading large files through UI can cause Metamask to crash #3

Open SKALE-jace opened 1 year ago

SKALE-jace commented 1 year ago

Bug/Issue

Large files (>4mb) send many permission requests, and potentially too many requests when uploading two or more simultaneously- causing Metamask to crash.

The bug has been repeatedly produced on fresh wallets.

To Reproduce

I created a static build of FS-UI to host on FileStorage.

Uploading the index.js(4.79mb) and index.css (6.63mb) simulanouesly in the assets directory of the static build caused Metamask to crash and be unable to restart.

Investigation of both browser-dev console and Metamask dev-console reveals wallet state is left with no response from RPC[1], and is stuck waiting for pending wallet_requestPermissions [2].

On every attempted reload of Metamask, the wallet crashes and ask to restart[3]

Occasionally, multiple versions of the same file appear in the upload window[4] (More than shown in screenshot and usually limited to the same number set for batchThreshold in presets.js in the config file)

Screenshots

[1]

1

[2]

2

[3]

3

[4]

4

Desktop

Additional Context

Bug does not exist when uploading same files through a script with filestorage.js, only UI

Temporary solution could be to limit the batchThreshold in presets.js from 20 to 1

Long term: use UploadStatus value to return complete before uploading next file

adiled commented 1 year ago

I'll add some context, the technical scope is filemanager.ts where all actions were initially set to be sequential using rxjs.

Point of investigation is piping within that module. Rxjs was used for flexibility here to allow other strategies when MTM could be made available. batchThreshold far as I recall is only a UX layer sugar for limits on file renaming.

This could be two separate issues, filemanager.ts can be tested in isolation, example use is in deploy script. Some thoughtful reorg between filestorage-ui/packages, admin-ui/network and external skale.js could also help with maintenance.


Prefixing a request to wallet could also help rule some things out: https://github.com/MetaMask/metamask-extension/issues/10584#issuecomment-972577382

.. And by way of that I recommend a migration from web3modal to rainbowkit or connectkit or other well-maintained library.

SKALE-jace commented 1 year ago

Thanks @adiled! I appreciate the deeper context. batchThreshold definitely doesn't fix the issue, but acts as a sidestep to prevent the conditions where it occurs.

In testing, this setting allowed only one upload through the UI at time.

For the limits on renaming are you referring to the maxFileDirNameLength in the type uploader in config.ts?

export type ConfigType = {
  optimize: {
    prefetchEvent: string; // placeholder (ignore)
    prefetchDepth: number; // (0, Infinity) directory depth to prefetch during navigation
  };
  branding: {
    logoUrl: string; // URL to logo image
    logoText: string; // Optional text placed next to logo
    greetingText: string;
  };
  navigator: {
    pageLimit: number; // max items per navigator page
  };
  uploader: {
    batchThreshold: number; // max items where upload is marked as batch
    maxFileDirNameLength: number; // max characters count of directory name
  };
  keys: {
    infuraId?: string;
    fortmaticKey?: string;
  },
  chains: {
    default?: boolean; // option to set as default
    protocol: string; // http or https
    nodeDomain: string; // node host FQDN
    version: string; // chain version
    sChainName: string; // chain name
    chainId: string; // chain ID
  }[]
}
adiled commented 1 year ago

@SKALE-jace By limit on renaming served by batchThreshold, I am referring to the UX where once you select files for upload, and then you have the option of renaming them if their count is less than batchThreshold. It is very much a misnomer, and has no influence on the underlying function of bulk upload.

For this exact issue though, it might be because a large file chunked up could be triggering several simultaneous transactions within one filemanager.ts atomic upload, and then if the wallet_requestPermissions is not yet called, multiple requests are likely to break it as in the referenced metamask issue (not ditto but likely applicable solution).

maxFileDirNameLength is for the maximum characters for any given file name, thinking of presets as a config only for UI layer might help.