Enhancement/1483 chainspacing #1496

adamgall · 2024-03-27T03:22:12Z

Closes #1483
Closes #1336

This PR implements chainspacing in the URL.

All URLs that exist in the context of a Safe have a network ID (same ones that the Safe UI uses) prefixed to the address.

Additionally, error page logic has been separated to clearly identify the two scenarios that might occur for a Safe being unable to be loaded:

The given address simply isn't a Safe on the expected (as per the network prefix) Safe.
The app is connected to a different network than what the Safe is expecting (as per the network prefix).

Additionally, stronger typing has been enforced in the many places were navigation links exist. This stronger typing required early-exiting rendering attempts where either the network prefix or address are not available yet (e.g. as a Safe is loading), so special review and testing should be taken to ensure that the logical changes around rendering links was implemented correctly.

Testing

Functional changes in this PR include:

Safes will only load if they include a network prefix in the new query param-based address.
automatically applying a network prefix to the address in the new query param-based url only when redirecting from the old path-based URL.
fixing all internal links and navigation in the app to apply and utilize the new query param-based safe addresses.
splitting the "can't load Safe" error screen into three distinct messages
- for when there is a problem with the format of the address query param
- for when the app is connected to a different network than the network prefix in the address query param
- for when the syntax and network are all good, but the Safe can't load because of actual loading issues, like the Safe API is down or the address just isn't an actual Safe.

adamgall

Left a handful of comments on some implementation details. Would love more eyes and brains on this.

src/constants/routes.ts

src/main.tsx

src/pages/DAOController.tsx

src/pages/daos/[daoAddress]/edit/governance/index.tsx

src/pages/daos/[daoAddress]/hierarchy/index.tsx

adamgall · 2024-03-27T04:22:27Z

src/components/ui/menus/DAOSearch/SearchDisplay.tsx

+            onClick={() => {
+              onClickView();
+              if (closeDrawer) closeDrawer();
+              action.resetDAO();
+              navigate(DAO_ROUTES.dao.relative(addressPrefix, address));
+            }}


Not super related to this PR, but I snuck this change in here anyway. Basically, made it so that only the button in the search result list is clickable, not the whole element.

src/components/pages/DaoSettings/components/Metadata/index.tsx

src/components/pages/DaoDashboard/Info/index.tsx

src/components/Proposals/ProposalCard/ProposalCard.tsx

netlify · 2024-03-27T13:43:20Z

✅ Deploy Preview for fractal-dev ready!

Name	Link
🔨 Latest commit	`9396909`
🔍 Latest deploy log	https://app.netlify.com/sites/fractal-dev/deploys/660723ff93476f00087479e9
😎 Deploy Preview	https://deploy-preview-1496.app.dev.fractalframework.xyz
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Da-Colon

Nice! Answer to your big question and one requested change in ManageDAOMenu

src/components/pages/DaoDashboard/Info/index.tsx

Da-Colon · 2024-03-28T03:07:30Z

src/components/ui/menus/FavoritesMenu/FavoritesList.tsx

@@ -45,6 +47,7 @@ export function FavoritesList() {
            {favoritesList.map(favorite => (
              <Favorite
                key={favorite}
+                network={addressPrefix}


We've talked before about revisiting LocalStorage setup. Should a card be made to start noting this?

src/components/ui/menus/ManageDAO/ManageDAOMenu.tsx

src/constants/routes.ts

src/hooks/DAO/proposal/useSubmitProposal.ts

src/hooks/DAO/useDAOController.ts

src/main.tsx

src/types/fractal.ts

adamgall

Left a couple new comments as per my most recent pushed work.

In general, the majority of my new updates tackle:

Fixing the unintentional bug I introduced in ManageDAOMenu (thanks @Da-Colon for finding that)
removing daoNetwork from Fractal state, and just using the value from useNetworkConfig instead, throughout the app.

src/hooks/DAO/loaders/useFractalNode.ts

src/hooks/DAO/useDAOController.ts

…x from the currently connected network

…o prefix the network to navigation

…ig everywhere

…tting errors

src/components/pages/DaoDashboard/Info/index.tsx

src/i18n/locales/en/common.json

adamgall · 2024-03-29T02:59:10Z

src/pages/DAOController.tsx

-function InvalidSafe() {
-  const { name } = useNetworkConfig();
-  const { t } = useTranslation('common');
-
-  return (
-    <Center
-      padding="3rem"
-      textColor="grayscale.100"
-    >
-      <VStack>
-        <Text
-          paddingTop="3rem"
-          textStyle="text-6xl-mono-regular"
-        >
-          {t('errorSentryFallbackTitle')}
-        </Text>
-        <Text>{t('invalidSafe1', { chain: name })}</Text>
-        <Text paddingBottom="1rem">{t('invalidSafe2')}</Text>
-        <Button onClick={() => window.location.reload()}>{t('refresh')}</Button>
-      </VStack>
-    </Center>
-  );
-}


Pulled into own File and made generic so we can throw any of the three error types at it

adamgall · 2024-03-29T02:59:30Z

src/pages/DAOController.tsx


 export default function DAOController() {
-  const { node } = useFractal();


Don't need this

adamgall · 2024-03-29T02:59:59Z

src/pages/DAOController.tsx

@@ -59,7 +32,6 @@ export default function DAOController() {
    return theme;
  }, [daoMetadata]);

-  const validSafe = node.safe;


Don't need this, we simply assume the address is a valid safe until it's not.

adamgall · 2024-03-29T03:14:29Z

src/hooks/DAO/loaders/useFractalNode.ts

+    if (addressPrefix + daoAddress !== currentValidSafe.current) {
+      setDAO(addressPrefix, daoAddress);
    }


And now, if we got here, we only want to call setDAO if we haven't done so yet for the given address

adamgall · 2024-03-29T03:25:38Z

src/i18n/locales/en/common.json

+  "invalidSafe2": "Please double check the address then try again.",
+  "wrongNetwork1": "Fractal is currently connected to {{chain}}.",
+  "wrongNetwork2": "Try switching the app to the correct chain for your Safe.",
+  "badQueryParam1": "The Safe address in the URL isn't formatted correctly",
+  "badQueryParam2": "",


Better error strings for the various kinds of initial loading issues that might occur

adamgall · 2024-03-29T03:28:16Z

src/hooks/DAO/loaders/useFractalNode.ts

+
+      let safeInfo;
+
+      try {
+        if (!safeAPI) throw new Error('SafeAPI not set');
+
+        safeInfo = await requestWithRetries(
+          () => safeAPI.getSafeInfo(utils.getAddress(_daoAddress)),
+          5,
+        );
+      } catch (e) {
+        reset({ error: true });
+        return;
      }
-      setNodeLoading(false);
+
+      if (!safeInfo) {
+        reset({ error: true });
+        return;
+      }
+
+      // if here, we have a valid Safe!
+
+      action.dispatch({
+        type: NodeAction.SET_FRACTAL_MODULES,
+        payload: await lookupModules(safeInfo.modules),
+      });
+
+      action.dispatch({
+        type: NodeAction.SET_SAFE_INFO,
+        payload: safeInfo,
+      });


This was mostly just a flattening of logic and moving all non-get-data stuff out of this file and back into useDAOController. This useFractalNode hook is now only used for fetching stuff, no more checking if input data is valid, if it's in here (and not undefined) we should use it to go start fetching.

adamgall · 2024-03-29T03:29:24Z

src/hooks/DAO/loaders/useFractalNode.ts

+    if (skip || addressPrefix === undefined || daoAddress === undefined) {
+      reset({ error: false });
+      return;


If skip is true, that means there was an error with the address or formatting or we're on the wrong network. If addressPrefix of daoAddress is undefined, then damn the App is still standing up. In either case, just reset state and GTFO.

adamgall · 2024-03-29T03:33:18Z

src/hooks/DAO/useDAOController.ts

+  const addressWithPrefix = searchParams.get('dao');
+  const validDaoQueryString = /^[^\s:]+:[^\s:]+$/;
+
+  const prefixAndAddress = addressWithPrefix?.split(':');
+  const addressPrefix = prefixAndAddress?.[0];
+  const daoAddress = prefixAndAddress?.[1];
+
+  const invalidQuery =
+    addressWithPrefix === null ||
+    !validDaoQueryString.test(addressWithPrefix) ||
+    !utils.isAddress(daoAddress || '');
+
+  const { addressPrefix: connectedAddressPrefix } = useNetworkConfig();
+  const wrongNetwork = addressPrefix !== connectedAddressPrefix;
+
+  const skip = invalidQuery || wrongNetwork;


This is fun. Do all the things needed to validate that:

we have a query string of the correct key

dao

the value of that key is formatted correctly

string-without-whitespace followed by ":" followed by string-without-whitespace

the ethereum address portion is formatted correctly

right length, checksum is valid if exists, etc

the network identified in the query param matches the network the app is currently on

so if the user is trying to load a sep:0xabc dao, the app must be on Sepolia

If any of those things fail, we don't try to load anything and show the appropriate error screen.

adamgall · 2024-03-29T03:36:06Z

@Da-Colon @mudrila @tomstuart123 I have just pushed a bunch of fixes and nice QoL improvements, this is ready for review again.

@Da-Colon i've addressed all of your comments and done more testing, things seem smooth as butter

tomstuart123 · 2024-03-29T12:12:16Z

Tried the new netlify link with 4 prior DAOs.

TLDR

All mainnet prior DAO links worked when on the right chain already and were reformated correctly
All sepolia prior dao links didn't work when on the right chain already and were given this

- When on sepolia (different chain), all mainnet prior DAO links saw the below message which I believe is the intended error message

Mainnet DAO links tried

Decent multisig (mainnet) - https://app.dev.fractalframework.xyz/daos/0xD26c85D435F02DaB8B220cd4D2d398f6f646e235
Decent's tokenvoting (mainnet) - https://app.dev.fractalframework.xyz/daos/0xB98d45F9021D71E6Fc30b43FD37FB3b1Bf12c064
Shutter's token voting (mainnet)- https://app.dev.fractalframework.xyz/daos/0x36bD3044ab68f600f6d3e081056F34f2a58432c4
AwakeVCs - https://app.dev.fractalframework.xyz/daos/0xdD6CeFA62239272f1eDf755ba6471eacb7DF2Fa5

Sepolia DAO links tried

Myosin multisig - https://app.dev.fractalframework.xyz/home?dao=0xdef90A94273a1A1A72B33D39129fa41E6C08Be3a
One of my sepolia test dao s- https://app.dev.fractalframework.xyz/home?dao=0xA287241240B5C8fce743Fb291D4F38cD669298d4

adamgall · 2024-03-29T12:15:13Z

@tomstuart123 the Sepolia links you tried are indeed formatted incorrectly. Notice how they are the new query params, but don't include the network prefix. This is invalid as per this new PR.

tomstuart123 · 2024-03-29T12:33:25Z

got ya. My bad for not double chekcing the new address. Forgot the new PR had gone to dev

Okay now retested with the links below

Sepolia
app.dev.fractalframework.xyz/daos/0xdef90A94273a1A1A72B33D39129fa41E6C08Be3a
app.dev.fractalframework.xyz/daos/0xA287241240B5C8fce743Fb291D4F38cD669298d4

All redirected well

I also managed to play with two of the error message

This appeared when being on ethereum using an old sepolia link

This appeared when being on ethereum using a new sepolia link

both should be working as intended so I'll give this an approve from me. Thanks @adamgall

…ent/1483-chainspacing

Da-Colon

Looks good thanks for the changes!

adamgall requested review from Da-Colon, mudrila and tomstuart123 March 27, 2024 03:22

adamgall linked an issue Mar 27, 2024 that may be closed by this pull request

Chainspacing the URL #1483

Closed

adamgall self-assigned this Mar 27, 2024

adamgall force-pushed the enhancement/1483-chainspacing branch 2 times, most recently from 307c8c7 to 789573e Compare March 27, 2024 03:36

adamgall linked an issue Mar 27, 2024 that may be closed by this pull request

Oops! Couldn't find a Safe... #1336

Closed

adamgall commented Mar 27, 2024

View reviewed changes

tomstuart123 approved these changes Mar 27, 2024

View reviewed changes

tomstuart123 self-requested a review March 27, 2024 13:19

Base automatically changed from enhancement/1492-dao-identifier-query-param to develop March 27, 2024 13:42

adamgall marked this pull request as ready for review March 27, 2024 13:43

Da-Colon requested changes Mar 28, 2024

View reviewed changes

adamgall force-pushed the enhancement/1483-chainspacing branch from acb4008 to b02998b Compare March 28, 2024 23:07

adamgall commented Mar 28, 2024

View reviewed changes

src/hooks/DAO/loaders/useFractalNode.ts Outdated Show resolved Hide resolved

src/hooks/DAO/loaders/useFractalNode.ts Outdated Show resolved Hide resolved

src/hooks/DAO/useDAOController.ts Outdated Show resolved Hide resolved

adamgall force-pushed the enhancement/1483-chainspacing branch 2 times, most recently from 4be13e7 to 86a01a9 Compare March 29, 2024 02:21

adamgall added 11 commits March 28, 2024 22:23

The search bar DAO selection click area is now just button

48d1a2d

Add an "addressPrefix" to the network config objects

2facb7d

When redirecting at initial load, best-guess append the network prefi…

2d03379

…x from the currently connected network

When using search input, and navigating to found address, make sure t…

aeb3f8e

…o prefix the network to navigation

Add daoNetwork to fractal state object, use it where appropriate

35766a4

Differentiate error pages between invalid DAO address, and wrong network

6e4a125

Update DAO_ROUTES.relative to take network prefix, fix all errors

d1f3759

Don't need to setErrorLoading(true) when wrong network identified

568167c

Reorder function params

c74e02a

Reorder function parameters

aff2c1e

Switch from returning <></> to null

65ca263

adamgall added 2 commits March 28, 2024 22:23

Remove "daoNetwork" from Fractal state, use value from useNetworkConf…

20f8ec0

…ig everywhere

Remove unused parameter

5525e40

adamgall force-pushed the enhancement/1483-chainspacing branch from 86a01a9 to 5525e40 Compare March 29, 2024 02:24

adamgall added 3 commits March 28, 2024 22:52

Improve logic around checking for connected network and address forma…

468a3fe

…tting errors

We want to show all loading info boxes when a DAO is loading

5aace1f

Move address formatting check out of useFractalnode

9447ffa

adamgall commented Mar 29, 2024

View reviewed changes

tomstuart123 approved these changes Mar 29, 2024

View reviewed changes

Merge commit 'bb5984d30f2366e6ba3f60220d415d54104474d6' into enhancem…

9396909

…ent/1483-chainspacing

Da-Colon approved these changes Mar 29, 2024

View reviewed changes

adamgall merged commit 785eff5 into develop Mar 29, 2024
7 checks passed

adamgall deleted the enhancement/1483-chainspacing branch March 29, 2024 20:30

adamgall mentioned this pull request Apr 5, 2024

Release/2024-04-08 Part 1: The Merge into prod #1521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/1483 chainspacing #1496

Enhancement/1483 chainspacing #1496

adamgall commented Mar 27, 2024 •

edited

Loading

adamgall left a comment

adamgall Mar 27, 2024

netlify bot commented Mar 27, 2024 •

edited

Loading

Da-Colon left a comment

Da-Colon Mar 28, 2024

adamgall left a comment

adamgall Mar 29, 2024

adamgall Mar 29, 2024

adamgall Mar 29, 2024

adamgall Mar 29, 2024

adamgall Mar 29, 2024

adamgall Mar 29, 2024

adamgall Mar 29, 2024

adamgall Mar 29, 2024

adamgall commented Mar 29, 2024 •

edited

Loading

tomstuart123 commented Mar 29, 2024

adamgall commented Mar 29, 2024

tomstuart123 commented Mar 29, 2024

Da-Colon left a comment


		export default function DAOController() {
		const { node } = useFractal();

Enhancement/1483 chainspacing #1496

Enhancement/1483 chainspacing #1496

Conversation

adamgall commented Mar 27, 2024 • edited Loading

Testing

adamgall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Mar 27, 2024 • edited Loading

✅ Deploy Preview for fractal-dev ready!

Da-Colon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall commented Mar 29, 2024 • edited Loading

tomstuart123 commented Mar 29, 2024

adamgall commented Mar 29, 2024

tomstuart123 commented Mar 29, 2024

Da-Colon left a comment

Choose a reason for hiding this comment

adamgall commented Mar 27, 2024 •

edited

Loading

netlify bot commented Mar 27, 2024 •

edited

Loading

adamgall commented Mar 29, 2024 •

edited

Loading