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

chore: initialize #3

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

Conversation

kingsley-einstein
Copy link

Resolves #1

.eslintrc Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Sep 26, 2024

Unused files (1)

src/scripts/custom-chains.ts

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Sep 26, 2024

@@ -0,0 +1,299 @@
export const ufaucetAbi = [
Copy link
Member

Choose a reason for hiding this comment

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

I would just make this a JSON file its cleaner that way.

Copy link
Author

Choose a reason for hiding this comment

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

I want to take advantage of typing this way.

package.json Show resolved Hide resolved
@kingsley-einstein kingsley-einstein marked this pull request as ready for review October 26, 2024 05:19
src/scripts/mint.ts Outdated Show resolved Hide resolved
src/scripts/redeem.ts Outdated Show resolved Hide resolved
@0x4007 0x4007 requested a review from rndquu October 27, 2024 12:57
@kingsley-einstein
Copy link
Author

@0x4007 I just realized that I was previously unassigned from this task. I already raised a PR.

Copy link

ubiquity-os bot commented Nov 8, 2024

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

@rndquu rndquu self-requested a review November 8, 2024 15:13
@kingsley-einstein
Copy link
Author

@rndquu

The listed criteria in #1 (comment) have been satisfied by the latest push.

Copy link

ubiquity-os bot commented Nov 10, 2024

@kingsley-einstein, this task has been idle for a while. Please provide an update.

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.

@kingsley-einstein

  1. Pls merge feat: add UBQ fund test script kingsley-einstein/uusd.ubq.fi#3
  2. Check this screenshot:
Screenshot 2024-11-11 at 19 03 59

Somehow when I enter 100 UUSD to mint the actually passed amount is 100000000 while the expected amount is 100000000000000000000. There's smth wrong with decimals. Pls check all over the code.

@rndquu rndquu marked this pull request as draft November 11, 2024 16:12
@kingsley-einstein
Copy link
Author

@kingsley-einstein

  1. Pls merge feat: add UBQ fund test script kingsley-einstein/uusd.ubq.fi#3
  2. Check this screenshot:
Screenshot 2024-11-11 at 19 03 59 Somehow when I enter 100 UUSD to mint the actually passed amount is 100000000 while the expected amount is 100000000000000000000. There's smth wrong with decimals. Pls check all over the code.

@rndquu
I'm on it now.

@kingsley-einstein kingsley-einstein marked this pull request as ready for review November 11, 2024 16:35
@kingsley-einstein
Copy link
Author

@rndquu
Changes have been made. It turns out I had hardcoded the decimals.

@zugdev
Copy link

zugdev commented Nov 11, 2024

User should be able to mint without specifying slippage, in that case you should pass in 0 for min and maxInt for max.

image

@zugdev
Copy link

zugdev commented Nov 11, 2024

Screenshot 2024-11-11 at 19 03 59

By the screenshot it looks like mint with governance value is either always true or inverted.

@zugdev
Copy link

zugdev commented Nov 11, 2024

When I fill the form initially the mint button is visible. Only after the allowance check runs it displays the approval button. The button should be disabled until allowance is checked, because if user proceeds to quickly press the mint button he will get reverted.

Screen.Recording.2024-11-11.180859.mp4

@zugdev
Copy link

zugdev commented Nov 11, 2024

Also, displaying the minting/redeeming fees is important. In the LUSD case, currently it's 0%, but you should read it dynamically from the collateralInfo function. It's vital as the fees are a part of our planned business model and will be non-null in near future. You can refer to the code I wrote for this: https://github.com/zugdev/uusd.ubq.fi/blob/f4c6623cec130cff75e299ca47f0a45b63ace819/src/pages/mint.ts#L48-L85

@zugdev
Copy link

zugdev commented Nov 11, 2024

The button seems to be generally a little inconsistent, it flickers:

Screen.Recording.2024-11-11.182203.mp4

I suggest on form update you check allowances to determine the button's state.

Copy link

@zugdev zugdev left a comment

Choose a reason for hiding this comment

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

@rndquu, I've left a few comments and suggestions. @kingsley-einstein, please wait for him to check them before you implement those, he might disagree with some.


const CLIENT_OR_ACCOUNT_ERROR = "Client or account not initialized";

export async function getAllCollaterals() {
Copy link

Choose a reason for hiding this comment

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

You are creating a contract instance and a public function for every single contract method, there is no need to do that. As in viem docs, you should make a public contract instance and then you can directly call it's methods. From their official documentation:

import { getContract } from 'viem'
import { wagmiAbi } from './abi'
import { publicClient, walletClient } from './client'
 
// 1. Create contract instance
const contract = getContract({
  address: '0xFBA3912Ca04dd458c843e2EE08967fC04f3579c2',
  abi: wagmiAbi,
  // 1a. Insert a single client
  client: publicClient,
  // 1b. Or public and/or wallet clients
  client: { public: publicClient, wallet: walletClient }
})
 
// 2. Call contract methods, fetch events, listen to events, etc.
const result = await contract.read.totalSupply()
const logs = await contract.getEvents.Transfer()
const unwatch = contract.watchEvent.Transfer(
  { from: '0xA0Cf798816D4b9b9866b5330EEa46a18382f251e' },
  { onLogs(logs) { console.log(logs) } }
)

let maxGovernanceIn = 0;
let isOneToOne = false;

let isButtonInteractionsDisabled = false;
Copy link

Choose a reason for hiding this comment

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

I don't think there is a need to have this variable as you can just use mintButton.disabled to check the status. This could be the cause for the button's inconsistency.

let dollarOutMin = 0;
let maxCollateralIn = 0;
let maxGovernanceIn = 0;
let isOneToOne = false;
Copy link

Choose a reason for hiding this comment

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

This should initially be true.


const collateralRecord: Record<string | number, `0x${string}`> = {};
const toastActions = new ToastActions();
const publicClient = createPublicClient({
Copy link

Choose a reason for hiding this comment

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

You are creating multiple clients. Why not share a single client with the entire app?

const web3Client = getConnectedClient();

if (allowanceButton !== null)
allowanceButton.disabled = web3Client === null || !collateralAddress || !web3Client.account || isButtonInteractionsDisabled;
Copy link

Choose a reason for hiding this comment

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

It would be prettier if you consolidated allowanceButton and mintButton into an actionButton, since they are never displayed concomitantly and share the same objective.

@@ -0,0 +1,259 @@
import { createPublicClient, http, parseUnits, BaseError } from "viem";
Copy link

Choose a reason for hiding this comment

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

The comments in mint.ts should make sense here too.

wallet.updateConnectButtonText("");

await wallet.connectIfAuthorized();
await mint.initCollateralList();
Copy link

Choose a reason for hiding this comment

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

The collateral list is shared between mint and redeem. Making it global is a good idea.

@@ -6,6 +6,6 @@ export default defineConfig({
// implement node event listeners here
},
experimentalStudio: true,
baseUrl: "http://localhost:8080",
baseUrl: "http://localhost:10090",
Copy link

Choose a reason for hiding this comment

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

8080 is the default HTML port, why change it?

}

//eslint-disable-next-line @typescript-eslint/no-explicit-any
export function toSf(i: any, sf: number = 3) {
Copy link

Choose a reason for hiding this comment

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

This naming isn't clear

<link rel="stylesheet" href="style/style.css" />
</head>
<body class="w-screen overflow-x-hidden min-h-screen drawer">
<main class="container mx-auto flex max-w-full lg:w-[80%] flex-col justify-start items-center py-4 px-4 lg:px-5 gap-12 overflow-x-hidden drawer-content">
Copy link

Choose a reason for hiding this comment

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

Styling in class name / Tailwind is not used within our UIs, tho I am not sure if it matters as styling will be made in a future PR

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.

Overall looks good.

@zugdev I've added some of your notes to a separate issue. Feel free to update it if there're any UI bugs. Regarding the comments on the code, I think they can be addressed in a separate PR.

Copy link

ubiquity-os bot commented Nov 16, 2024

@kingsley-einstein, this task has been idle for a while. Please provide an update.

@kingsley-einstein
Copy link
Author

@kingsley-einstein, this task has been idle for a while. Please provide an update.

Still under review

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.

Create UI for UbiquityPool
5 participants