Add withdraw functionality #281
Conversation
- add withdraw button for eth, lords, beasts, GTs and bloberts - add balances of these in the profile dialog - add test withdrawal for 1 beast
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a new asynchronous function Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (2)
ui/src/app/components/profile/ProfileDialog.tsx (1)
125-125: Ensurebeastsis notundefinedbefore accessinglengthWhile
beastsis expected to be an array, there might be scenarios where it isundefinedornull, leading to a runtime error when accessingbeasts.length. To prevent this, add a null check.Modify the code as follows:
- <p>{beasts.length}</p> + <p>{beasts?.length ?? 0}</p>This change safely handles cases where
beastsmay not be defined by displaying0instead.ui/src/app/lib/utils/syscalls.ts (1)
1555-1556: Remove unnecessary console log statementThe
console.logstatement is likely leftover from debugging and should be removed in production code to avoid unnecessary console output.- console.log(calls[calls.length - 1]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- ui/src/app/api/getBeasts.ts (1 hunks)
- ui/src/app/components/profile/ProfileDialog.tsx (10 hunks)
- ui/src/app/lib/utils/syscalls.ts (2 hunks)
- ui/src/app/page.tsx (5 hunks)
🧰 Additional context used
🔇 Additional comments (11)
ui/src/app/page.tsx (6)
7-7: New import added for getBeasts function.The
getBeastsfunction is now imported from the API. This is likely related to the new functionality for fetching and displaying beast data.
99-99: New state variable added for beasts.A new state variable
beastsis initialized using theuseStatehook. This will be used to store the fetched beast data.
481-489: New function added to fetch beasts.The
handleFetchBeastsfunction is implemented to fetch beast data using the importedgetBeastsfunction. This is a good addition to support the new beast-related functionality.
489-493: Updated useEffect hook to fetch beasts.The useEffect hook now includes a call to
handleFetchBeasts(). This ensures that beast data is fetched when the component mounts or when the address or network changes.
943-945: Updated ProfileDialog props.The ProfileDialog component now receives
goldenTokens,beasts, andblobertsDataas props. This allows the component to display information about these assets.
Line range hint
1-954: Summary of changes: Beast functionality added successfully.The changes in this file successfully implement the new beast-related functionality:
- A new import for the
getBeastsfunction is added.- A new state variable
beastsis introduced to store the fetched beast data.- A
handleFetchBeastsfunction is implemented to fetch the beast data.- The useEffect hook is updated to call
handleFetchBeastswhen necessary.- The ProfileDialog component is updated to receive the new beast data as props.
These changes are consistent with the PR objective of adding withdraw functionality, specifically for beasts. The implementation appears to be clean and well-integrated into the existing codebase.
ui/src/app/api/getBeasts.ts (2)
35-36: Verify the necessity of parsingbeast.tokenIdwithJSON.parse.Using
JSON.parseonbeast.tokenIdassumes thattokenIdis a JSON-formatted string. IftokenIdis already a primitive type (e.g., a string or number), parsing may not be necessary and could cause errors. Confirm the format oftokenIdto ensure correct parsing.To verify the format of
tokenId, you can inspect sample data with the following script:#!/bin/bash # Description: Check the format of `tokenId` in beast objects. # Test: Retrieve sample `tokenId` values. Expect: `tokenId` to be a JSON string if parsing is required. rg --json -g '*.json' '"tokenId":' | jq '.tokenId'
15-15:⚠️ Potential issueEnsure
networkis defined before using non-null assertion!.The non-null assertion operator
!is used onnetwork(network!) to assert that it is notnullorundefined. To prevent potential runtime errors, ensure thatnetworkis always defined whengetBeastsis called, or include a validation check before its usage.To verify all calls to
getBeastsprovide a validnetwork, you can run the following script:ui/src/app/components/profile/ProfileDialog.tsx (1)
180-182: Verify that thewithdrawfunction handles the newbeastsparameter correctlyThe
withdrawfunction signature has been updated to include thebeastsparameter. Ensure that all usages of this function pass the correct arguments and that the function implementation correctly processes thebeastsarray.To confirm that the
withdrawfunction is updated appropriately, you can search the codebase for its implementations:This script searches for the
withdrawfunction definition and its usages to ensure consistency across the codebase.ui/src/app/lib/utils/syscalls.ts (2)
1543-1545: Re-enable withdrawals of ETH and Lords tokens if requiredThe code for transferring ETH and Lords tokens is commented out. If withdrawing these assets is intended, consider uncommenting and integrating these transactions.
Confirm whether the commented code for transferring ETH and Lords tokens should be active. If so, update the
callsarray accordingly.let calls = []; - // if (ethBalance > BigInt(0)) { - // calls.push(transferEthTx); - // } - // if (lordsBalance > BigInt(0)) { - // calls.push(transferLordsTx); - // } + if (ethBalance > BigInt(0)) { + calls.push(transferEthTx); + } + if (lordsBalance > BigInt(0)) { + calls.push(transferLordsTx); + }Also, ensure that
transferEthTxandtransferLordsTxare properly defined and that the contracts support these operations.Also applies to: 1547-1549
1529-1539: Validate beast IDs before processingEnsure that the beast IDs provided are valid and owned by the user to prevent unintended errors or security issues.
Add validation logic before creating transfer transactions:
// Create a transaction for each beast ID + // Validate beast IDs + const ownedBeasts = await getOwnedBeasts(account.address); + const validBeasts = beasts.filter((beastId) => ownedBeasts.includes(beastId)); + if (validBeasts.length !== beasts.length) { + throw new Error("One or more beast IDs are invalid or not owned by the user."); + } + const transferBeastTxs = validBeasts.map((beastId) => ({Implement
getOwnedBeaststo retrieve the list of beast IDs owned by the user.Alternatively, you can verify ownership by running the following shell script:
| const recursiveFetch: any = async ( | ||
| beasts: any[], | ||
| nextPageKey: string | null | ||
| ) => { |
There was a problem hiding this comment.
🛠️ 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
};| beasts = beasts.concat( | ||
| data?.nfts?.map((beast: any) => { | ||
| const tokenId = JSON.parse(beast.tokenId); | ||
| return Number(tokenId); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
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);
})
);
}| } catch (ex) { | ||
| console.log("error fetching beasts", ex); | ||
| } |
There was a problem hiding this comment.
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;
}| ) => Promise<void>; | ||
| ethBalance: bigint; | ||
| lordsBalance: bigint; | ||
| ethContractAddress: string; | ||
| lordsContractAddress: string; | ||
| goldenTokens: number[]; | ||
| beasts: number[]; | ||
| blobertsData: any; |
There was a problem hiding this comment.
🛠️ 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;| <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> |
There was a problem hiding this comment.
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 ethersImport 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.
ui/src/app/lib/utils/syscalls.ts
Outdated
| const { transaction_hash } = await account.execute( | ||
| calls[calls.length - 1] | ||
| ); |
There was a problem hiding this comment.
Execute all withdrawal transactions instead of only the last one
Currently, the code executes only the last transaction in the calls array, which means that if there are multiple beasts to withdraw, only the last beast will be processed.
To fix this, you should execute all transactions in the calls array. You can achieve this by passing the entire calls array to account.execute().
Apply this diff to execute all transactions:
- const { transaction_hash } = await account.execute(
- calls[calls.length - 1]
- );
+ const { transaction_hash } = await account.execute(
+ calls
+ );📝 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.
| const { transaction_hash } = await account.execute( | |
| calls[calls.length - 1] | |
| ); | |
| const { transaction_hash } = await account.execute( | |
| calls | |
| ); |
| if (beasts.length > 0) { | ||
| calls.push(...transferBeastTxs); | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| 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."); | |
| } |
| const transferBeastTxs = beasts.map((beastId) => ({ | ||
| contractAddress: beastsContract?.address ?? "", | ||
| entrypoint: "transfer_from", | ||
| calldata: [ | ||
| account.address, | ||
| adminAccountAddress, | ||
| beastId.toString(), | ||
| "0x0", | ||
| ], | ||
| })); |
There was a problem hiding this comment.
🛠️ 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.
| setIsWithdrawing(false); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
ui/src/app/components/profile/ProfileDialog.tsx (4)
2-2: LGTM! Consider defining a type forbloberts.The new imports and interface changes align well with the added functionality for golden tokens, beasts, and bloberts. However, to improve type safety, consider defining a specific type for
blobertsinstead of usingany[].You could define a type like this:
type Blobert = { // Add relevant properties here }; // Update the interface bloberts: Blobert[];This will provide better type checking and code completion for
bloberts.Also applies to: 26-31, 37-39
77-80: LGTM! Consider using object destructuring for network config.The addition of
tournamentEndedflag and network-specific addresses is well-implemented. To slightly improve readability, you could use object destructuring for the network config.Consider refactoring like this:
const { goldenTokenAddress, tournamentWinnerAddress: blobertsAddress } = networkConfig[network!];This makes it clearer that both addresses are coming from the same config object.
Line range hint
184-197: Consider adding a confirmation step and visual feedback for withdrawals.The updated withdraw functionality now includes the new assets (golden tokens, beasts, bloberts) which is great. However, to enhance user experience and prevent accidental withdrawals, consider the following improvements:
- Add a confirmation dialog before initiating the withdrawal.
- Provide visual feedback during the withdrawal process (e.g., a loading spinner).
- Show a success or error message after the withdrawal attempt.
Here's a basic example of how you could implement this:
const [isWithdrawing, setIsWithdrawing] = useState(false); const [withdrawResult, setWithdrawResult] = useState<'success' | 'error' | null>(null); const handleWithdraw = async () => { if (window.confirm('Are you sure you want to withdraw all assets?')) { setIsWithdrawing(true); try { await withdraw( controllerDelegate, account!, ethBalance, lordsBalance, goldenTokenAddress, goldenTokens, beasts, blobertsAddress, blobertsData?.tokens, tournamentEnded ); setWithdrawResult('success'); } catch (error) { console.error('Withdrawal failed:', error); setWithdrawResult('error'); } finally { setIsWithdrawing(false); } } }; // In your JSX: <Button size={"lg"} onClick={handleWithdraw} disabled={isWithdrawing || controllerDelegate === "0x0" || controllerDelegate === ""} > {isWithdrawing ? 'Withdrawing...' : 'Withdraw'} </Button> {withdrawResult === 'success' && <p>Withdrawal successful!</p>} {withdrawResult === 'error' && <p>Withdrawal failed. Please try again.</p>}This implementation adds a confirmation step, shows a loading state, and provides feedback on the withdrawal result.
152-152: Consider improving layout consistency and accessibility.The layout adjustments to accommodate new elements are good, and the responsive design choices (hiding certain elements on smaller screens) are appropriate. However, there are a few areas where consistency and accessibility could be improved:
Consistency in text sizes: Some headings use
text-2xl sm:text-4xl, while others only usetext-2xl. Consider standardizing this across all headings.Accessibility: Ensure that all interactive elements (buttons, clickable areas) have appropriate ARIA labels for screen readers.
Color contrast: Verify that the text colors (especially
text-terminal-green) have sufficient contrast against the background for readability.Here are some suggestions:
- Standardize heading sizes:
<h2 className="text-terminal-green text-2xl sm:text-4xl uppercase m-0"> Withdraw </h2>
- Add ARIA labels to buttons:
<Button aria-label="Copy address" onClick={handleCopy} > Copy </Button>
- Verify color contrast:
// Consider using a tool to check the contrast ratio between these colors <h1 className="text-terminal-green">These small improvements will enhance the overall user experience and accessibility of the component.
Also applies to: 208-208
ui/src/app/lib/utils/syscalls.ts (1)
1593-1593: Remove console log statement used for debuggingThe
console.log(calls);statement is likely intended for debugging purposes and should be removed to prevent unnecessary console output in production, which can clutter logs or potentially expose sensitive information.Apply this diff to remove the debug statement:
- console.log(calls);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ui/src/app/components/profile/ProfileDialog.tsx (10 hunks)
- ui/src/app/lib/utils/syscalls.ts (2 hunks)
🧰 Additional context used
| <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> | ||
| </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> |
There was a problem hiding this comment.
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.
| goldenTokens: number[], | ||
| beasts: number[], | ||
| blobertsAddress: string, | ||
| bloberts: any[], |
There was a problem hiding this comment.
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[],
Summary by CodeRabbit
Release Notes
New Features
ProfileDialogcomponent to display additional information about golden tokens and beasts.Bug Fixes
These updates improve user experience by providing more comprehensive asset management and data visibility.