Settings safe integration #327
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@pheuberger is attempting to deploy a commit to the Hypercerts Foundation Team on Vercel. A member of the Team first needs to authorize it. |
pheuberger
force-pushed
the
settings-safe-integration
branch
from
da34af3 to
e5290c6
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
pheuberger
force-pushed
the
settings-safe-integration
branch
from
e5290c6 to
e8908ee
Compare
bitbeckers
reviewed
Dec 19, 2024
components/accounts/account-item.tsx
Outdated
| import { truncateEthereumAddress } from "@/lib/utils"; | ||
|
|
||
| interface AccountItemProps { | ||
| type: "personal" | "safe"; |
There was a problem hiding this comment.
to stick with the evm lingo, it's an 'eoa' not 'personal'
pheuberger
force-pushed
the
settings-safe-integration
branch
from
e8908ee to
0313a2f
Compare
This patch adds API Kit which unfortunately overrides viem's Address type and thus introduces build errors around code that was and still is fine. Please read this GitHub issue for more details: safe-global/safe-core-sdk#1087 To get the app to build again I needed to assert address to be of type `0x${string}` in a few places. We know it's the correct type, it's just that the TypeScript compiler doesn't, so this is safe to do.
The motivation for the account selector is to enable users of the Hypercerts app to execute certain transactions via a Safe. Currently only the connected wallet can issue transactions. The modal will fetch the Safe accounts that the currently connected wallet is a member of and display them alongside the currently connected wallet. Any selection made will just visually change the currently connected wallet, but will have no impact on the behavior of the app when sending transactions just yet. Future commits will fully implement this feature.
The chain display component emits the following warning: Image with src "/chain-icons/ethereum_sepolia.png" has "fill" but is missing "sizes" prop. Please add it to improve page performance. This patch adds the sizes prop to both image tags.
The component was emitting two warnings: Image with src "/avatar-default.jpg" has "fill" but is missing "sizes" prop. Please add it to improve page performance. Image with src "/avatar-default.jpg" has "fill" and parent element with invalid "position". Provided "static" should be one of absolute,fixed,relative. This patch adds the sizes prop and makes the parent AvatarFallback relative.
This is to silence the lint warnings.
This addresses this lint warning: Using `<img>` could result in slower LCP and higher bandwidth. Consider using `<Image />` from `next/image` to automatically optimize images. This may incur additional usage or cost from your provider.
Without this patch, when the user pastes an invalid URL or wants to type one out themselves the app crashes. This patch adds a check if the avatar url is valid and only then renders the avatar preview.
The dialog steps were fairly complicated strings that were repeated multiple times. It's easy to introduce a typo into these strings, so they were replaced with constants.
This is in preparation to enabling smart account message signing via Safes. That way we can abstract the selection of EOA or smart account into this hook. Another advantage is that we don't need to construct the domain on every signature request.
pheuberger
force-pushed
the
settings-safe-integration
branch
from
0313a2f to
bc93fdf
Compare
This patch adds support for the currently selected account to initiate a change of user data (display name and image uri). The message is written out the Safe Transaction Service where users can facilitate the signature flow. Then the safe message hash is sent to the Hypercerts API which keeps track of the signature request. Since this flow is so different to the EOA signing, it can't reuse the EOA signing flow and at the end of the transaction it can't make the change to the user data right away, which makes this a 1-step flow.
Without this patch the user doesn't know that a multisig user settings update is waiting to be signed. This displays a message below the form and informs the user of the new name and image url.
This patch adds a way for users to cancel a pending signature request. This is so that they don't have to commit a change to the database if it contains a mistake or was made in error.
Managing loading state is scattered all throughout this component and thus hard to follow. Some terms (pending) were clashing, so this patch does away with isPending in favor isLoading while still keeping the finer grained loading flags.
This patch adds a refresh button to the pending signature card that, when pressed, will refresh the cached GraphQL data to see if the pending request was executed and apply the name and avatar changes to the form. This patch extracts the pending update card into its own component so as to not overload the settings form too much.
Without this patch submitting the user settings changes will not re-trigger a GraphQL fetch of the user data. This is important, however, as we need the message hash of the Safe message in the case a multisig is being used for this data change. As it turns out, revalidatePathServerAction() isn't needed for this and on its own won't work. The most straight forward approach is invoking refetch() from useQuery().
When using a Safe to sign a user data change, the submission of the request will display a pending data card. Given the data is pending and explicitly shown, we need to display the original data that's still the de facto correct data until the pending data change is executed. Also invert the if statement to remove one level of nesting.
pheuberger
force-pushed
the
settings-safe-integration
branch
from
bc93fdf to
ace376b
Compare
|
@bitbeckers I implemented the changes and fixed the bug I was facing. Everything builds locally and I re-deployed the Vercel preview. I hope the PR will recognize that. Seems as though Vercel doesn't pick up my force-pushed changes, so when I redeploy it will redeploy the previously failing commit. I might have to create a new PR. Unless you have a better idea? |
Just kicked the Vercel deployment from my end. If it indeed doesn't work, it'd need a new PR yeah |
It seems the build completed successfully. Feel free to merge any time :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a set of features to enable Safe multisig support to make changes to the profile name and avatar image in
/settings.In a fast-follow, I'll implement an affordance to allow the user to refresh the pending request to see if the threshold of the Safe was met and commit the change to the database