-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: remove wallet bridge #202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge UX improvement. Want to make sure the messages are accurate before approving.
<div className="w-screen max-w-7xl m-auto"> | ||
{error ? ( | ||
<> | ||
<div className="flex justify-end flex-row space-x-2 items-center mr-6 m-2"> | ||
{networkDropdown} | ||
</div> | ||
<div>Error connecting to chain</div> | ||
<div>Error connecting to chain!</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
@@ -130,7 +120,7 @@ const CloseVaultDialog = ({ | |||
{...noticeProps} | |||
className="overflow-hidden text-interOrange" | |||
> | |||
Offer sent to Agoric Smart Wallet for approval. | |||
Vault has been closed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only been sent, right? The transaction could still fail. For example if another transaction in flight took out more debt, this pay down to close would not be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh just the variable name is off. Approving assuming you'll revise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the variable name
@@ -111,7 +113,7 @@ const ManageVaults = () => { | |||
); | |||
} else if (!chainConnection) { | |||
popupContent = 'Connect your wallet to manage your vaults.'; | |||
} else if (vaults?.size === 0) { | |||
} else if (vaults?.size === 0 || isSmartWalletProvisioned === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider having its own case message. Perhaps "You have no Agoric smart wallet yet."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that but I'm leaning towards keeping this as is. We already show the dialog, so it's not necessary to tell them twice, and it doesn't matter to them that there's no smart wallet until they actually try to create a vault, where they'll see the "Provision" button in that view.
|
||
const body = ( | ||
<span> | ||
To interact with contracts on the Agoric chain, a smart wallet must be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future work: should we have a utility for this in ui-kit? Worth an issue imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly for the provisioning fee, though I'm not sure how likely that flow is to change (i.e. if we implement automatic provisioning). We already have a utility for the provision status. I'm not sure if the whole dialog flow is worth making into a component depending on how dapps want to handle it
d247168
to
f9a0084
Compare
fixes #201