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

Add withdraw functionality #281

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions ui/src/app/api/getBeasts.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { Network } from "@/app/hooks/useUIStore";
import { networkConfig } from "@/app/lib/networkConfig";
import { indexAddress } from "@/app/lib/utils";

export const getBeasts = async (
owner: string,
beastsAddress: string,
network: Network
): Promise<number[]> => {
const recursiveFetch: any = async (
beasts: any[],
nextPageKey: string | null
) => {
Comment on lines +10 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Specify explicit types for recursiveFetch.

Currently, the recursiveFetch function and its parameters are typed with any, which reduces the benefits of TypeScript's type safety. Consider defining explicit types for the function and its parameters to enhance code reliability and maintainability.

You can update the function signature as follows:

const recursiveFetch = async (
  beasts: number[],
  nextPageKey: string | null
): Promise<number[]> => {
  // function body
};

let url = `${
networkConfig[network!].blastUrl
}/builder/getWalletNFTs?contractAddress=${beastsAddress}&walletAddress=${indexAddress(
owner
).toLowerCase()}&pageSize=100`;

if (nextPageKey) {
url += `&pageKey=${nextPageKey}`;
}

try {
const response = await fetch(url, {
method: "GET",
headers: {
"Content-Type": "application/json",
},
});

const data = await response.json();
beasts = beasts.concat(
data?.nfts?.map((beast: any) => {
const tokenId = JSON.parse(beast.tokenId);
return Number(tokenId);
})
);
Comment on lines +33 to +38
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle cases where data.nfts might be undefined.

The code attempts to map over data?.nfts, but if data.nfts is undefined, this could lead to unexpected behavior. Ensure that data.nfts is an array before mapping to prevent errors.

You can safely handle this scenario by adding a conditional check:

if (Array.isArray(data?.nfts)) {
  beasts = beasts.concat(
    data.nfts.map((beast: any) => {
      const tokenId = JSON.parse(beast.tokenId);
      return Number(tokenId);
    })
  );
}


if (data.nextPageKey) {
return recursiveFetch(beasts, data.nextPageKey);
}
} catch (ex) {
console.log("error fetching beasts", ex);
}
Comment on lines +43 to +45
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error handling in the catch block.

Currently, the catch block logs the error but allows the function to return potentially incomplete data. Consider throwing the error after logging to ensure that calling functions are aware of the failure and can handle it appropriately.

Update the catch block to re-throw the error:

} catch (ex) {
  console.log("error fetching beasts", ex);
  throw ex;
}


return beasts;
};

let beastsData = await recursiveFetch([], null);

return beastsData;
};
76 changes: 69 additions & 7 deletions ui/src/app/components/profile/ProfileDialog.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
import { Button } from "@/app/components/buttons/Button";
import { CartridgeIcon } from "@/app/components/icons/Icons";
import { CartridgeIcon, GiBruteIcon } from "@/app/components/icons/Icons";
import useAdventurerStore from "@/app/hooks/useAdventurerStore";
import useNetworkAccount from "@/app/hooks/useNetworkAccount";
import { useQueriesStore } from "@/app/hooks/useQueryStore";
import useUIStore from "@/app/hooks/useUIStore";
import { checkCartridgeConnector } from "@/app/lib/connectors";
import { networkConfig } from "@/app/lib/networkConfig";
import { copyToClipboard, displayAddress, padAddress } from "@/app/lib/utils";
import { NullAdventurer } from "@/app/types";
import CartridgeConnector from "@cartridge/connector";
import { useConnect, useDisconnect } from "@starknet-react/core";
import Image from "next/image";
import Eth from "public/icons/eth.svg";
import Lords from "public/icons/lords.svg";
import { useState } from "react";
import { MdClose } from "react-icons/md";
import { AccountInterface } from "starknet";
Expand All @@ -18,12 +22,21 @@ interface ProfileDialogprops {
adminAccountAddress: string,
account: AccountInterface,
ethBalance: bigint,
lordsBalance: bigint
lordsBalance: bigint,
goldenTokenAddress: string,
goldenTokens: number[],
beasts: number[],
blobertsAddress: string,
bloberts: any[],
tournamentEnded: boolean
) => Promise<void>;
ethBalance: bigint;
lordsBalance: bigint;
ethContractAddress: string;
lordsContractAddress: string;
goldenTokens: number[];
beasts: number[];
blobertsData: any;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Define a specific type for blobertsData instead of using any

Using the any type for blobertsData bypasses TypeScript's type checking, which can lead to runtime errors that are difficult to debug. To enhance type safety and code maintainability, consider defining an explicit interface for blobertsData.

For example, you can create an interface like:

interface BlobertsData {
  tokens: TokenType[]; // Replace `TokenType` with the actual token structure
  // Add other relevant properties here
}

Update the prop type accordingly:

- blobertsData: any;
+ blobertsData: BlobertsData;

}

export const ProfileDialog = ({
Expand All @@ -32,6 +45,9 @@ export const ProfileDialog = ({
lordsBalance,
ethContractAddress,
lordsContractAddress,
goldenTokens,
beasts,
blobertsData,
}: ProfileDialogprops) => {
const { setShowProfile, setNetwork } = useUIStore();
const { disconnect } = useDisconnect();
Expand All @@ -43,6 +59,7 @@ export const ProfileDialog = ({
const username = useUIStore((state) => state.username);
const controllerDelegate = useUIStore((state) => state.controllerDelegate);
const handleOffboarded = useUIStore((state) => state.handleOffboarded);
const network = useUIStore((state) => state.network);
const { connector } = useConnect();

const handleCopy = () => {
Expand All @@ -57,6 +74,11 @@ export const ProfileDialog = ({
setTimeout(() => setCopiedDelegate(false), 2000);
};

const tournamentEnded = process.env.NEXT_PUBLIC_TOURNAMENT_ENDED === "true";

const goldenTokenAddress = networkConfig[network!].goldenTokenAddress;
const blobertsAddress = networkConfig[network!].tournamentWinnerAddress;

return (
<div className="fixed w-full h-full sm:w-3/4 sm:h-3/4 top-0 sm:top-1/8 bg-terminal-black border border-terminal-green flex flex-col items-center p-10 z-30">
<button
Expand All @@ -68,7 +90,7 @@ export const ProfileDialog = ({
<MdClose size={50} />
</button>
<div className="flex flex-col items-center h-full gap-5">
<div className="flex flex-col items-center">
<div className="flex flex-col gap-2 items-center">
{checkCartridgeConnector(connector) && (
<CartridgeIcon className="w-10 h-10 fill-current" />
)}
Expand All @@ -90,10 +112,44 @@ export const ProfileDialog = ({
{displayAddress(address!)}
</h3>
)}
<div className="flex flex-row gap-5">
<span className="flex flex-row items-center">
<Eth className="self-center w-6 h-6 fill-current mr-1" />
<p>{(Number(ethBalance) / 10 ** 18).toFixed(4).toString()}</p>
</span>
<span className="flex flex-row items-center">
<Lords className="self-center w-6 h-6 fill-current mr-1" />
<p>{(Number(lordsBalance) / 10 ** 18).toFixed(2).toString()}</p>
Comment on lines +118 to +122
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid precision loss when converting bigint balances to Number

Converting ethBalance and lordsBalance from bigint to Number may cause precision loss for large values, resulting in inaccurate balance displays. To handle large integers safely, consider using a big number library like ethers.js.

First, install ethers if it's not already a dependency:

npm install ethers

Import the formatUnits function:

import { formatUnits } from "ethers/lib/utils";

Then, update the balance displays:

- <p>{(Number(ethBalance) / 10 ** 18).toFixed(4).toString()}</p>
+ <p>{parseFloat(formatUnits(ethBalance, 18)).toFixed(4)}</p>

- <p>{(Number(lordsBalance) / 10 ** 18).toFixed(2).toString()}</p>
+ <p>{parseFloat(formatUnits(lordsBalance, 18)).toFixed(2)}</p>

This ensures that large balances are formatted correctly without losing precision.

</span>
<span className="flex flex-row gap-1 items-center">
<Image
src={"/golden-token.png"}
alt="golden-tokens"
width={32}
height={32}
/>
<p>{goldenTokens?.length}</p>
</span>
<span className="flex flex-row items-center">
<GiBruteIcon className="self-center w-8 h-8 fill-current mr-1 text-terminal-yellow" />
<p>{beasts.length}</p>
</span>
{tournamentEnded && (
<span className="flex flex-row gap-1 items-center">
<Image
src={"/blobert.png"}
alt="bloberts"
width={32}
height={32}
/>
<p>{blobertsData?.tokens.length}</p>
</span>
)}
</div>
Comment on lines +115 to +148
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve balance display and consider using a big number library.

The new UI elements for displaying various asset balances greatly enhance the user experience. However, there's a potential issue with precision loss when converting bigint balances to Number for ETH and LORDS.

To address this, consider using a big number library like ethers.js for formatting. Here's an example of how you could refactor:

import { formatUnits } from "ethers/lib/utils";

// ...

<p>{parseFloat(formatUnits(ethBalance, 18)).toFixed(4)}</p>
<p>{parseFloat(formatUnits(lordsBalance, 18)).toFixed(2)}</p>

This approach ensures accurate representation of large balances without precision loss.

The display of golden tokens, beasts, and bloberts (when the tournament has ended) is well-implemented and provides a clear overview of the user's assets.

</div>
<div className="flex flex-col sm:flex-row items-center justify-center gap-5 sm:gap-2">
{checkCartridgeConnector(connector) && (
<div className="flex flex-col items-center border border-terminal-green p-2 sm:p-5 text-center sm:gap-10 z-1 h-[200px] sm:h-[400px] sm:w-1/3">
<div className="flex flex-col items-center border border-terminal-green p-2 sm:p-5 text-center gap-5 sm:gap-10 z-1 h-[300px] sm:h-[400px] sm:w-1/3">
<h2 className="text-terminal-green text-2xl sm:text-4xl uppercase m-0">
Withdraw
</h2>
Expand Down Expand Up @@ -121,7 +177,7 @@ export const ProfileDialog = ({
(connector as unknown as CartridgeConnector).openMenu()
}
>
Change Settings
Set Delegate
</Button>
</div>
<Button
Expand All @@ -131,7 +187,13 @@ export const ProfileDialog = ({
controllerDelegate,
account!,
ethBalance,
lordsBalance
lordsBalance,
goldenTokenAddress,
goldenTokens,
beasts,
blobertsAddress,
blobertsData?.tokens,
tournamentEnded
)
}
disabled={
Expand All @@ -143,7 +205,7 @@ export const ProfileDialog = ({
</div>
)}
{checkCartridgeConnector(connector) && (
<div className="flex flex-col items-center border border-terminal-green p-5 text-center sm:gap-5 z-1 sm:h-[400px] sm:w-1/3">
<div className="hidden sm:flex flex-col items-center border border-terminal-green p-5 text-center sm:gap-5 z-1 sm:h-[400px] sm:w-1/3">
<h2 className="text-terminal-green text-2xl sm:text-4xl uppercase m-0">
Topup
</h2>
Expand Down
86 changes: 68 additions & 18 deletions ui/src/app/lib/utils/syscalls.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1508,40 +1508,89 @@ export function createSyscalls({
adminAccountAddress: string,
account: AccountInterface,
ethBalance: bigint,
lordsBalance: bigint
lordsBalance: bigint,
goldenTokenAddress: string,
goldenTokens: number[],
beasts: number[],
blobertsAddress: string,
bloberts: any[],
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Specify a concrete type instead of any[] for bloberts

The parameter bloberts: any[] uses the any type, which reduces type safety and defeats the purpose of using TypeScript. Defining a specific interface or type for bloberts will enhance code reliability and maintainability.

Consider defining an interface for bloberts, for example:

interface Blobert {
  tokenId: number;
  /* other properties */
}

And update the function signature:

- bloberts: any[],
+ bloberts: Blobert[],

tournamentEnded: boolean
) => {
try {
setIsWithdrawing(true);

const transferEthTx = {
contractAddress: ethContract?.address ?? "",
entrypoint: "transfer",
calldata: [adminAccountAddress, ethBalance ?? "0x0", "0x0"],
calldata: [adminAccountAddress, ethBalance.toString() ?? "0x0", "0x0"],
};

const transferLordsTx = {
contractAddress: lordsContract?.address ?? "",
entrypoint: "transfer",
calldata: [adminAccountAddress, lordsBalance ?? "0x0", "0x0"],
calldata: [
adminAccountAddress,
lordsBalance.toString() ?? "0x0",
"0x0",
],
};

// const maxFee = getMaxFee(network!);
const transferGoldenTokenTxs = goldenTokens.map((goldenTokenId) => ({
contractAddress: goldenTokenAddress ?? "",
entrypoint: "transfer_from",
calldata: [
account.address,
adminAccountAddress,
goldenTokenId.toString(),
"0x0",
],
}));

const transferBeastTxs = beasts.map((beastId) => ({
contractAddress: beastsContract?.address ?? "",
entrypoint: "transfer_from",
calldata: [
account.address,
adminAccountAddress,
beastId.toString(),
"0x0",
],
}));
Comment on lines +1549 to +1558
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider batch executing transfers for better efficiency

Creating individual transactions for each beast could be inefficient and may lead to performance issues. Consider batching the transfers if the contract supports it.

If the beastsContract supports a batch transfer method, you can modify the code as follows:

-      const transferBeastTxs = beasts.map((beastId) => ({
-        contractAddress: beastsContract?.address ?? "",
-        entrypoint: "transfer_from",
-        calldata: [
-          account.address,
-          adminAccountAddress,
-          beastId.toString(),
-          "0x0",
-        ],
-      }));
+      const transferBeastsTx = {
+        contractAddress: beastsContract?.address ?? "",
+        entrypoint: "batch_transfer_from",
+        calldata: [
+          account.address,
+          adminAccountAddress,
+          beasts.length.toString(),
+          ...beasts.map((beastId) => [beastId.toString(), "0x0"]).flat(),
+        ],
+      };

Ensure that batch_transfer_from is implemented in the beastsContract.

Committable suggestion was skipped due to low confidence.


const transferBlobertTxs = bloberts.map((blobert) => ({
contractAddress: blobertsAddress ?? "",
entrypoint: "transfer_from",
calldata: [
account.address,
adminAccountAddress,
blobert.tokenId.toString(),
"0x0",
],
}));

let calls = [];

// const transferEthTx = {
// contractAddress: ethContract?.address ?? "",
// entrypoint: "transfer",
// calldata: CallData.compile([
// masterAccountAddress,
// newEthBalance ?? "0x0",
// "0x0",
// ]),
// };
if (ethBalance > BigInt(0)) {
calls.push(transferEthTx);
}

if (lordsBalance > BigInt(0)) {
calls.push(transferLordsTx);
}

if (goldenTokens.length > 0) {
calls.push(...transferGoldenTokenTxs);
}

// If they have Lords also withdraw
const calls =
lordsBalance > BigInt(0)
? [transferEthTx, transferLordsTx]
: [transferEthTx];
if (beasts.length > 0) {
calls.push(...transferBeastTxs);
}

Comment on lines +1585 to +1588
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle empty withdrawal requests gracefully

If the beasts array is empty, the calls array will also be empty, leading to an error when trying to access calls[calls.length - 1].

Add a check to ensure that there are transactions to execute:

       if (beasts.length > 0) {
         calls.push(...transferBeastTxs);
       }

+      if (calls.length === 0) {
+        throw new Error("No assets to withdraw.");
+      }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (beasts.length > 0) {
calls.push(...transferBeastTxs);
}
if (beasts.length > 0) {
calls.push(...transferBeastTxs);
}
if (calls.length === 0) {
throw new Error("No assets to withdraw.");
}

if (bloberts.length > 0 && tournamentEnded) {
calls.push(...transferBlobertTxs);
}

console.log(calls);

const { transaction_hash } = await account.execute(calls);

Expand All @@ -1557,6 +1606,7 @@ export function createSyscalls({
getBalances();
} catch (error) {
console.error(error);
setIsWithdrawing(false);
throw error;
}
Comment on lines +1609 to 1611
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure setIsWithdrawing(false) is called in all code paths

In the catch block, after rethrowing the error, the setIsWithdrawing(false) call might not execute if an error occurs. This could leave the application in a loading state.

Consider moving setIsWithdrawing(false) to a finally block to ensure it executes regardless of whether an error occurs.

       setIsWithdrawing(false);
       throw error;
+    } finally {
+      setIsWithdrawing(false);
     }

Committable suggestion was skipped due to low confidence.

};
Expand Down
23 changes: 19 additions & 4 deletions ui/src/app/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import EthBalanceFragment from "@/app/abi/EthBalanceFragment.json";
import Game from "@/app/abi/Game.json";
import Lords from "@/app/abi/Lords.json";
import Pragma from "@/app/abi/Pragma.json";
import { getBeasts } from "@/app/api/getBeasts";
import { getGoldenTokens } from "@/app/api/getGoldenTokens";
import { DeathDialog } from "@/app/components/adventurer/DeathDialog";
import Player from "@/app/components/adventurer/Player";
Expand Down Expand Up @@ -95,6 +96,7 @@ function Home() {
(state) => state.updateAdventurerStats
);
const [goldenTokens, setGoldenTokens] = useState<number[]>([]);
const [beasts, setBeasts] = useState<number[]>([]);
const calls = useTransactionCartStore((state) => state.calls);
const screen = useUIStore((state) => state.screen);
const setScreen = useUIStore((state) => state.setScreen);
Expand Down Expand Up @@ -275,11 +277,11 @@ function Home() {

useEffect(() => {
const init = async () => {
const username = await (
connector as unknown as CartridgeConnector
).username();
const cartridgeConnector = connector as unknown as CartridgeConnector;
const username = await cartridgeConnector.username();
const delegate = await cartridgeConnector.delegateAccount();
setUsername(username || "");
setControllerDelegate("");
setControllerDelegate(delegate || "");
};
if (connector?.id.includes("controller")) {
setIsController(true);
Expand Down Expand Up @@ -476,8 +478,18 @@ function Home() {
setGoldenTokens(goldenTokens);
};

const handleFetchBeasts = async () => {
const beasts = await getBeasts(
address ?? "",
networkConfig[network!].beastsAddress,
network
);
setBeasts(beasts);
};

useEffect(() => {
handleFetchGoldenTokens();
handleFetchBeasts();
}, [address, network]);

const blobertTokenVariables = useMemo(() => {
Expand Down Expand Up @@ -928,6 +940,9 @@ function Home() {
lordsBalance={lordsBalance}
ethContractAddress={ethContract!.address}
lordsContractAddress={lordsContract!.address}
goldenTokens={goldenTokens}
beasts={beasts}
blobertsData={blobertsData}
/>
</div>
)}
Expand Down
Loading