1931 Part 2: 1710 Cross network favorites
#1975
Conversation
- update view components for new setup
✅ Deploy Preview for decent-interface-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Da-Colon
changed the base branch from
develop
to
issue/1931-local-storage-updates
…address makes better app. - Also fixed spots missing addressPrefix update
Da-Colon
changed the title
1710 Part 2: Cross network favorites1710 Part 2: Cross network favorites
Da-Colon
changed the title
1710 Part 2: Cross network favorites1930 Part 2: 1710 Cross network favorites
Da-Colon
changed the title
1930 Part 2: 1710 Cross network favorites1931 Part 2: 1710 Cross network favorites
|
Found a much better solution. (kinda a duh. moment). Soo. one thing is missing here. pulling the DAO's name on a different network. I have it almost setup but the last thing is targeting the right registry contract. I tried to find a way to make this work but ultimately I started rubbing up against the updates @adamgall is making in the typechain rework. I need those updates ha! then this will be a piece of cake. |
| * @todo Should switch to object for props | ||
| */ | ||
| const useDisplayName = (account?: string | null, truncate?: boolean) => { | ||
| const useDisplayName = (account?: string | null, truncate?: boolean, chainId?: number) => { |
There was a problem hiding this comment.
I would prefer these args be an object but didn't want to fill up this review with this change to all the other files
| <Image src={getNetworkIcon(network)} /> | ||
| <Show above="md"> | ||
| <Text>{networkConfig.chain.name}</Text> | ||
| <Text>{getChainName(network)}</Text> |
There was a problem hiding this comment.
I feel like these (and getChainIdFromPrefix usage above) could be wrapped a useMemo, where we could gracefully handle exceptions and return some acceptable default/error data.
…ao/fractal-interface into issue/1710-cross-network-favorites
src/utils/address.ts
Outdated
| export const decodePrefixedAddress = (address: string) => { | ||
| const [prefix, addressWithoutPrefix] = address.split(':'); | ||
| return { | ||
| network: prefix, | ||
| address: addressWithoutPrefix, | ||
| }; | ||
| }; | ||
|
|
||
| export const encodePrefixedAddress = (address: Address, network: string) => { | ||
| return `${network}:${address}`; | ||
| }; |
There was a problem hiding this comment.
Ok yes! This is a great start, but I also wanted to see some validation in here.
Like, addressWithoutPrefix will be undefined if address doesn't contain a ":".
There was a problem hiding this comment.
ah I see. wanted a clean return instead of undefined but I can do that.
There was a problem hiding this comment.
I think if you throw it doesn't add undefined to the return type
|
As far as the throw errors. I don't believe it will EVER hit here. Unsupported networks are caught way up the food chain. I did this more or less so I didn't have to type our a |
Which comment was this in reference to? |
these comments |
| <Image src={getNetworkIcon(network)} /> | ||
| <Show above="md"> | ||
| <Text>{networkConfig.chain.name}</Text> | ||
| <Text>{getChainName(network)}</Text> |
There was a problem hiding this comment.
I feel like these (and getChainIdFromPrefix usage above) could be wrapped a useMemo, where we could gracefully handle exceptions and return some acceptable default/error data.
So similar to my answer to @adamgall is this should never throw here. As the app won't let you connect to a network that isn't supported and since we are directly using the same configuration files. So IMO, its not needed. But if yall (@adamgall had similar thoughts) are not satisfied with my answe, then I can figure out the changes needed to make this happen, Though hard to test since the app itself won't let you switch to an unsupported network.
|
Try/catching errors like this (ones that we know "should never happen") are precisely the point. I'd like us to get into the habit of building more bulletproof code. If there's a possibility for a line of code to throw (when evaluating it without any extra context) (and there is in this instance) let's catch it and bubble it up. |
|
@Da-Colon j/w, do you plan on doing anything more with this PR? https://decentdao.slack.com/archives/C06QZ68L5SA/p1718121588983869 |
|
@adamgall Yessir. I'm creating a Placing an error Boundary with a Fallback UI, around Where this might fail.
|
@decentdao/product @decentdao/design Yall good with the look and copy? |
|
@DarksightKellar I didn't add the |
There was a problem hiding this comment.
@DarksightKellar I didn't add the
useMemoas suggested with the error boundary this handles this error as is, wdyt?
@Da-Colon Yhup, me likey.
Fixes #1710
Kinda a weird one to fit in the middle but wanted this work to be done prior to the Local Storage migration actually is done so that the typing is setup.
This PR handles adding the network prefix to the cached favorite string. It also adds some helper functions to get the network prefix and chain id via the network prefix to allow for the UI around the favorites.