Skip to content
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: add an approval/funding UI #30

Open
wants to merge 53 commits into
base: development
Choose a base branch
from

Conversation

zugdev
Copy link

@zugdev zugdev commented Nov 21, 2024

Resolves #29

I like the word funding better than approval, allowance or permit2, as it is better aligned with the "ubiquity banking" concept. Don't worry bout the text, it still needs rephrasing.

Current UI:

image

I have a couple questions:

  1. Should I force UUSD or should I offer an input field for token. If I should force, what about chains where UUSD is not at currently? I've included all permit2 chains I could, for now.

  2. I don't want to have user authenticate with GitHub, it's an unnecessary step, but if I did I could read what chain he has payments setup in. Is there a way to read what token he is using as reward too? If that was the case I should consider adding GitHub auth.

  3. I am waiting on the above to integrate ethers.

@zugdev zugdev requested a review from rndquu as a code owner November 21, 2024 03:26
@zugdev zugdev requested a review from 0x4007 November 21, 2024 03:26
Copy link

Unused dependencies (4)

Filename dependencies
package.json @coinbase/wallet-sdk
@reown/appkit
@reown/appkit-adapter-ethers5
ethers

Unused devDependencies (2)

Filename devDependencies
package.json @types/react
react

@rndquu rndquu requested review from rndquu and removed request for rndquu November 21, 2024 08:25
@rndquu
Copy link
Member

rndquu commented Nov 21, 2024

Should I force UUSD or should I offer an input field for token. If I should force, what about chains where UUSD is not at currently? I've included all permit2 chains I could, for now.

There should be a separate input for a token. When mainnet or gnosis is selected the UUSD should be set in the input token field by default but user should have the ability to change the token address.

I don't want to have user authenticate with GitHub, it's an unnecessary step, but if I did I could read what chain he has payments setup in. Is there a way to read what token he is using as reward too? If that was the case I should consider adding GitHub auth.

Check this comment. Overall github auth is not necessary as a part of the current github issue and can be solved in another PR.

@rndquu rndquu marked this pull request as draft November 21, 2024 08:43
Copy link

ubiquity-os bot commented Nov 22, 2024

@zugdev, this task has been idle for a while. Please provide an update.

@ubiquity-os ubiquity-os bot closed this Nov 24, 2024
@zugdev zugdev reopened this Nov 24, 2024
@0x4007
Copy link
Member

0x4007 commented Nov 24, 2024

  1. Should I force UUSD or should I offer an input field for token. If I should force, what about chains where UUSD is not at currently? I've included all permit2 chains I could, for now.

I wonder if it makes sense for us to bridge over to as many chains as we can. I suppose anybody can do this and we just need to aggregate the addresses.

  1. I don't want to have user authenticate with GitHub, it's an unnecessary step, but if I did I could read what chain he has payments setup in. Is there a way to read what token he is using as reward too? If that was the case I should consider adding GitHub auth.

This is actually determined by the partner project currently. So in theory we could have partner A, B, and C and each pays with their own preferred token on their own preferred chain. This is because they need to fund their wallets with whatever asset they want.

  1. I am waiting on the above to integrate ethers.

@zugdev
Copy link
Author

zugdev commented Nov 26, 2024

I think aside from providers that's it. I need to find a smart way to use providers in all 10+ chains, why don't we use Alchemy or some service like that?

@zugdev zugdev marked this pull request as ready for review November 26, 2024 21:10
@0x4007
Copy link
Member

0x4007 commented Dec 25, 2024

@rndquu can you move this review forward?

@rndquu rndquu requested review from rndquu and removed request for rndquu December 26, 2024 06:42
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zugdev

  1. Pls fix failing CI workflows
  2. Could you open etherscan transaction URL on transaction click?
Screenshot 2024-12-26 at 09 48 24 3. Somehow on revoking approval the "Success" modal is not shown although transaction is sent successfully

.github/workflows/build.yml Outdated Show resolved Hide resolved
@rndquu rndquu marked this pull request as draft December 26, 2024 07:09
@zugdev zugdev marked this pull request as ready for review December 26, 2024 07:45
@zugdev
Copy link
Author

zugdev commented Dec 26, 2024

@rndquu fixed

@rndquu rndquu self-requested a review December 26, 2024 13:10
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine

Copy link

@EresDev EresDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Just a few small improvements and we are good to go.

static/handle-approval.ts Outdated Show resolved Hide resolved
static/index.html Show resolved Hide resolved
@zugdev
Copy link
Author

zugdev commented Dec 26, 2024

@0x4007

Copy link

ubiquity-os bot commented Dec 29, 2024

@zugdev, this task has been idle for a while. Please provide an update.

@ubiquity-os ubiquity-os bot marked this pull request as draft December 29, 2024 21:18
@rndquu rndquu marked this pull request as ready for review December 31, 2024 10:35
Copy link

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you get it working with Anvil or did you test it on testnet? I tried with Anvil fork of mainnet but got Internal JSON-RPC error when trying to approve

static/populate-dropdown.ts Outdated Show resolved Hide resolved
test-dashboard.md Outdated Show resolved Hide resolved
Comment on lines +86 to +99
if (appState.getIsConnectedState() && window.ethereum) {
const ethereum = window.ethereum as ethers.providers.ExternalProvider;
if (ethereum.request) {
await ethereum.request({ method: "eth_requestAccounts" });
}

// Create a Web3Provider from window.ethereum
web3Provider = new ethers.providers.Web3Provider(window.ethereum);

// web3Provider signer will handle transaction signing
userSigner = web3Provider.getSigner(appState.getAddress());

console.log("User address:", await userSigner.getAddress());
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will only work for browser extension wallets but Reown also supports mobile wallets, so why not just use appState.getWalletProvider() and appState.subscribeProvider(handleChange)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that, but reown doesnt offer a signer so we would need to externally manage it anyway. also this is the standar for other UIs in ubiquity too

Copy link

@whilefoo whilefoo Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. appState.getWalletProvider() returns a provider and then you call provider.getSigner() and you get the signer

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appState.getWalletProvider() returns undefined, you can try it for urself :(

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reown was previously wallet connect, but there are still distinctions like that from a new product

console.error("Provider is not initialized");
return;
}

const tokenAddress = addressInput.value;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might wanna check if the address is ERC20 because if put a random address then it throws a revert error and it can be confusing to the users

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

managed that, thanks

@@ -1,6 +1,8 @@
import { appState, explorersUrl } from "./main";

export function renderErrorInModal(error: Error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for common errors like ACTION_REJECTED you can display a more user-friendly error message

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already display action rejected in a pretty way, also we try to avoid all errors instead of better displaying them. insufficient funds, missing new, numeric fault .. are handled. we could handle nonce id if user waits tooooo long to sign tho

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be pretty enough for a developer but it's not really pretty a normal user though

@@ -26,22 +21,32 @@ const metadata = {
url: "https://onboard.ubq.fi",
icons: ["https://avatars.githubusercontent.com/u/76412717"],
};
export const providersUrl: { [key: string]: string } = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add anvil network to providers and permit2 addresses otherwise it doesn't work when testing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you dont need to actually sign the transaction to see if it works, if the transaction to approve x amount pops up in ur wallet u know it works

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but btw I just checked and I've included anvil already, u can pick from te drop down

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I can select the network but it doesn't work because it's not in providersUrl. I wanted to try to see the success message and I can't get there

Comment on lines 62 to 65
<div class="current-allowance">
<span>current allowance:</span>
<span>Current allowance:</span>
<span class="current-allowance-amount">-</span>
</div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not too much work can you also display balance, I think it's beneficial for the user to know.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it too, but allowance is not tied to balance, so I figured its not necessary, lmk what you think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowance is not tied to balance

Normally, I've seen that the default requested allowance would be the user's balance amount.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but that's usually in a flow like swapping.

  1. user MUST allow 100 UUSD to swap 100 UUSD into 100 DAI.
  2. it's not a problem if he allows 119238129319203, its actually common for max allowances to happen (which is not good, opsec wise)

in our case having a balance is not mandatory, I can allow 10k UUSD even if I dont have that quantity in balance. but I can add if you guys want it

Comment on lines 50 to 61
approveButton.disabled = true;
revokeButton.disabled = true;

if (isAddressValid && isConnected) {
void getCurrentAllowance();
const isSuccess = await getCurrentAllowance();
if (isSuccess) {
approveButton.disabled = !isAmountValid;
revokeButton.disabled = false;
addressInput.style.border = green;
} else {
addressInput.style.border = red;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now every time I click out of the amount input, the buttons get disabled until allowance is fetched and it's a weird UX

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but thats what you meant by checking if its an erc20 token, should I add a spinner to indicate loading perhaps?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you will do the check when approve button is clicked and then it would show the error modal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add permit2 approval
5 participants