feat: Limit Orders #3347
feat: Limit Orders #3347
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
|
Caution Review failedThe pull request is closed. WalkthroughThis update enhances the decentralized finance application by introducing efficient retrieval of active orders and order book denominations, supported by optimized caching strategies. New procedures facilitate various interactions with order books, while improved user interfaces for placing limit orders and reviewing order history significantly enhance user engagement. Collectively, these enhancements create a more responsive trading environment, greatly improving the overall user experience. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OrderbookRouter
participant ActiveOrdersQuery
participant OrderbookService
User->>OrderbookRouter: Requests active orders
OrderbookRouter->>ActiveOrdersQuery: Fetch active orders
ActiveOrdersQuery->>OrderbookService: getOrderbookActiveOrders
OrderbookService-->>ActiveOrdersQuery: Return active orders data
ActiveOrdersQuery-->>OrderbookRouter: Return transformed active orders
OrderbookRouter-->>User: Display active orders
User->>OrderbookRouter: Requests maker fee
OrderbookRouter->>OrderbookService: getMakerFee
OrderbookService-->>OrderbookRouter: Return maker fee data
OrderbookRouter-->>User: Display maker fee
This diagram illustrates the interactions when a user requests active orders and maker fees, showcasing the flow from user requests through the order book service to the final display of information. 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 as PR comments)
Additionally, you can add 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: 39
Outside diff range and nitpick comments (13)
packages/web/pages/index.tsx (1)
Line range hint
93-99: Ensure SVGs have appropriate accessibility attributes.The SVGs used here do not have a title or alternative text, which is important for accessibility. Users who rely on screen readers will not receive any information about these images.
- <svg className="absolute h-full w-full lg:hidden" pointerEvents="none" viewBox="0 0 1300 900" height="900" preserveAspectRatio="xMidYMid slice"> + <svg className="absolute h-full w-full lg:hidden" pointerEvents="none" viewBox="0 0 1300 900" height="900" preserveAspectRatio="xMidYMid slice" role="img" aria-label="Description of the image">packages/trpc/src/assets.ts (1)
Line range hint
215-237: Remove unnecessaryelseclause.The
elseclause in thegetUserMarketAssetmethod is unnecessary because all branches before it either return or throw an exception. Removing it simplifies the control flow and improves readability.- } else { - return { - ...userAsset, - ...userMarketAsset, - }; - } + return { + ...userAsset, + ...userMarketAsset, + };packages/math/src/pool/concentrated/tick.ts (1)
Line range hint
385-387: Remove unnecessary else clause to simplify control flow.The else clause after the if condition checking for negative tick remainder is unnecessary because the previous branches have already handled all other conditions.
- } else if (tickRemainder.gt(new Int(0))) { + if (tickRemainder.gt(new Int(0))) { tick = tick.sub(tickRemainder); if (isLowerTick) { tick = tick.add(tickSpacingInt); } }packages/web/components/swap-tool/index.tsx (6)
Line range hint
365-388: Add keyboard accessibility to clickable elements.The button elements triggered by mouse events should also be accessible via keyboard events to ensure the application is accessible to users who rely on keyboard navigation.
+ onKeyUp={handleKeyUp}
Line range hint
393-412: Ensure keyboard accessibility for all interactive elements.Similar to previous findings, this segment also lacks keyboard event handlers for actions triggered by mouse clicks, which can hinder accessibility for keyboard-only users.
+ onKeyPress={handleKeyPress}
Line range hint
979-980: Remove redundant Boolean conversion.The use of
Booleanhere is unnecessary as the expression will naturally coerce to a boolean in a conditional statement.- Boolean(swapState.error) || + swapState.error ||
Line range hint
389-389: Specify button type to prevent unintended form submissions.It's a best practice to explicitly set the
typeattribute for button elements to avoid unexpected behaviors like form submission when not intended.+ type="button"
Line range hint
598-619: Explicitly define button type for clarity and safety.Defining the button type is crucial to prevent it from defaulting to
submitwhich can cause forms to be submitted unintentionally.+ type="button"
Line range hint
761-773: Add type attribute to button to avoid default submit behavior.Without specifying the type, buttons inside forms default to
submit, which might lead to unintended form submissions.+ type="button"packages/web/hooks/use-swap.tsx (4)
Line range hint
200-200: Remove redundant Boolean call.The
Booleanfunction call is redundant here since the value will already be coerced into a boolean by the logical operators.- Boolean(swapAssets.fromAsset) && Boolean(swapAssets.toAsset); + swapAssets.fromAsset && swapAssets.toAsset;
Line range hint
278-421: Avoid using async function as Promise executor.Using an async function as a Promise executor can lead to subtle bugs due to the way JavaScript handles asynchronous execution and error management. It's recommended to handle asynchronous operations outside the executor or use other constructs like async/await for better control flow and error handling.
const sendTradeTokenInTx = useCallback(async () => { try { // Extracted async logic here } catch (error) { // Handle errors } }, [dependencies]);
Line range hint
515-515: Remove redundant Boolean call.Similar to a previous comment, the
Booleanfunction call is redundant here and can be removed for cleaner code.- Boolean(quoteForCurrentBalance); + quoteForCurrentBalance;
Line range hint
569-573: Remove unnecessary else clauses.The else clauses in these blocks can be omitted because the preceding if conditions either return or break the flow, making the else unnecessary and potentially confusing.
- if (condition) { - return something; - } else { - return somethingElse; - } + if (condition) { + return something; + } + return somethingElse;Also applies to: 606-610
| @@ -51,6 +51,7 @@ import { useWalletSelect } from "./use-wallet-select"; | |||
| import { useQueryParamState } from "./window/use-query-param-state"; | |||
|
|
|||
| export type SwapState = ReturnType<typeof useSwap>; | |||
| export type SwapAsset = ReturnType<typeof useSwapAsset>["asset"]; | |||
There was a problem hiding this comment.
Consider using explicit types for better maintainability.
It's a good practice to explicitly define return types for exported types or interfaces to enhance code maintainability and readability, especially in a TypeScript project.
export type SwapAsset = ReturnType<typeof useSwapAsset>["asset"];| export function tickToPrice(tickIndex: Int): Dec { | ||
| if (tickIndex.isZero()) { | ||
| return new Dec(1); | ||
| } | ||
|
|
||
| const geometricExponentIncrementDistanceInTicks = nine.mul( | ||
| powTenBigDec(new Int(exponentAtPriceOne).neg()).toDec() | ||
| ); | ||
|
|
||
| if (tickIndex.lt(minTick) || tickIndex.gt(maxTick)) { | ||
| throw new Error( | ||
| `tickIndex is out of range: ${tickIndex.toString()}, min: ${minTick.toString()}, max: ${maxTick.toString()}` | ||
| ); | ||
| } | ||
|
|
||
| const geometricExponentDelta = new Dec(tickIndex) | ||
| .quoTruncate(new Dec(geometricExponentIncrementDistanceInTicks.truncate())) | ||
| .truncate(); | ||
|
|
||
| let exponentAtCurTick = new Int(exponentAtPriceOne).add( | ||
| geometricExponentDelta | ||
| ); | ||
| if (tickIndex.lt(new Int(0))) { | ||
| exponentAtCurTick = exponentAtCurTick.sub(new Int(1)); | ||
| } | ||
|
|
||
| const currentAdditiveIncrementInTicks = powTenBigDec(exponentAtCurTick); | ||
|
|
||
| const numAdditiveTicks = tickIndex.sub( | ||
| geometricExponentDelta.mul( | ||
| geometricExponentIncrementDistanceInTicks.truncate() | ||
| ) | ||
| ); | ||
|
|
||
| const price = powTenBigDec(geometricExponentDelta) | ||
| .add(new BigDec(numAdditiveTicks).mul(currentAdditiveIncrementInTicks)) | ||
| .toDec(); | ||
|
|
||
| if (price.gt(maxSpotPrice) || price.lt(minSpotPrice)) { | ||
| throw new Error( | ||
| `price is out of range: ${price.toString()}, min: ${minSpotPrice.toString()}, max: ${maxSpotPrice.toString()}` | ||
| ); | ||
| } | ||
|
|
||
| return price; | ||
| } |
There was a problem hiding this comment.
Refactor suggestion: Consider modularizing repetitive code blocks in tickToPrice.
The tickToPrice function contains repetitive code blocks, especially in the calculations involving geometric exponents and price bounds checking. This could be abstracted into smaller, reusable functions to improve code maintainability and readability.
+ function calculateGeometricExponentDelta(tickIndex, geometricExponentIncrementDistanceInTicks) {
+ return new Dec(tickIndex)
+ .quoTruncate(new Dec(geometricExponentIncrementDistanceInTicks.truncate()))
+ .truncate();
+ }
+
+ function checkTickIndexBounds(tickIndex) {
+ if (tickIndex.lt(minTick) || tickIndex.gt(maxTick)) {
+ throw new Error(
+ `tickIndex is out of range: ${tickIndex.toString()}, min: ${minTick.toString()}, max: ${maxTick.toString()}`
+ );
+ }
+ }
+
+ function checkPriceBounds(price) {
+ if (price.gt(maxSpotPrice) || price.lt(minSpotPrice)) {
+ throw new Error(
+ `price is out of range: ${price.toString()}, min: ${minSpotPrice.toString()}, max: ${maxSpotPrice.toString()}`
+ );
+ }
+ }
- if (tickIndex.lt(minTick) || tickIndex.gt(maxTick)) {
- throw new Error(
- `tickIndex is out of range: ${tickIndex.toString()}, min: ${minTick.toString()}, max: ${maxTick.toString()}`
- );
- }
- if (price.gt(maxSpotPrice) || price.lt(minSpotPrice)) {
- throw new Error(
- `price is out of range: ${price.toString()}, min: ${minSpotPrice.toString()}, max: ${maxSpotCommittable suggestion was skipped due to low confidence.
| @@ -2,7 +2,7 @@ import { Dec, Int } from "@keplr-wallet/unit"; | |||
|
|
|||
| import { approxSqrt } from "../../../utils"; | |||
| import { maxSpotPrice, maxTick, minSpotPrice } from "../const"; | |||
| import { priceToTick, tickToSqrtPrice } from "../tick"; | |||
| import { priceToTick, tickToPrice, tickToSqrtPrice } from "../tick"; | |||
There was a problem hiding this comment.
Optimize imports in test file.
The import tickToSqrtPrice is not used in the provided test cases. Consider removing it to keep the code clean and efficient.
- import { priceToTick, tickToPrice, tickToSqrtPrice } from "../tick";
+ import { priceToTick, tickToPrice } from "../tick";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.
| import { priceToTick, tickToPrice, tickToSqrtPrice } from "../tick"; | |
| import { priceToTick, tickToPrice } from "../tick"; |
| )} | ||
| <LimitTradeDetails swapState={swapState} /> | ||
| {!account?.isWalletConnected ? ( | ||
| <Button |
There was a problem hiding this comment.
Ensure buttons have explicit types to avoid unintentional form submissions.
The button here should have an explicit type to prevent it from submitting a form if it happens to be within one. This is a common issue that can lead to unexpected behavior in React applications.
- <Button onClick={() =>
+ <Button type="button" onClick={() =>Committable suggestion was skipped due to low confidence.
| const { orderbooks, isLoading } = { | ||
| // TODO: Replace with SQS filtered response | ||
| orderbooks: testnetOrderbooks, |
There was a problem hiding this comment.
Consider replacing hardcoded values with dynamic data fetching.
The orderbooks are currently hardcoded for testnet environments. This might not be suitable for production. Consider fetching these dynamically based on the network environment. Also, enhance the TODO comment to specify what needs to be replaced more clearly.
- // TODO: Replace with SQS filtered response
- orderbooks: testnetOrderbooks,
+ // TODO: Replace hardcoded orderbooks with dynamic fetching based on the network environment
+ orderbooks: getDynamicOrderbooks(),Committable suggestion was skipped due to low confidence.
| export function getOrderbookTickState({ | ||
| orderbookAddress, | ||
| chainList, | ||
| tickIds, | ||
| }: { | ||
| orderbookAddress: string; | ||
| chainList: Chain[]; | ||
| tickIds: number[]; | ||
| }) { | ||
| return cachified({ | ||
| cache: tickInfoCache, | ||
| key: `orderbookTickInfo-${orderbookAddress}-${tickIds | ||
| .sort((a, b) => a - b) | ||
| .join(",")}`, | ||
| ttl: 1000 * 60 * 2, // 2 minutes | ||
| getFreshValue: () => | ||
| queryOrderbookTicks({ orderbookAddress, chainList, tickIds }).then( | ||
| ({ data }) => data.ticks | ||
| ), |
There was a problem hiding this comment.
Ensure consistent caching and error handling in tick state retrieval functions.
Both getOrderbookTickState and getOrderbookTickUnrealizedCancels functions use a similar pattern for caching and fetching data. It's crucial to ensure that the caching keys are constructed consistently and that errors during data fetching are handled appropriately to prevent runtime crashes.
+ }).catch(error => {
+ console.error('Failed to fetch tick information:', error);
+ throw error; // Rethrow or handle as needed
+ })Also applies to: 35-55
| export const OrderHistory = observer(() => { | ||
| const { accountStore } = useStore(); | ||
| const wallet = accountStore.getWallet(accountStore.osmosisChainId); | ||
|
|
||
| // In the future we need to merge in the past orders | ||
| const { orders, isLoading } = useOrderbookAllActiveOrders({ | ||
| userAddress: wallet?.address ?? "", | ||
| }); | ||
|
|
||
| const table = useReactTable<Order>({ | ||
| data: orders, | ||
| columns: tableColumns, | ||
| getCoreRowModel: getCoreRowModel(), | ||
| }); | ||
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <div className="flex items-center justify-center py-10"> | ||
| <Spinner className="!h-10 !w-10" /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div className="mt-3 flex flex-col"> | ||
| <table> | ||
| <thead className="border-b border-osmoverse-700"> | ||
| {table.getHeaderGroups().map((headerGroup) => ( | ||
| <tr className="!border-0 bg-transparent" key={headerGroup.id}> | ||
| {headerGroup.headers.map((header) => ( | ||
| <th key={header.id}> | ||
| {flexRender( | ||
| header.column.columnDef.header, | ||
| header.getContext() | ||
| )} | ||
| </th> | ||
| ))} | ||
| </tr> | ||
| ))} | ||
| </thead> | ||
| <tbody className="bg-transparent"> | ||
| {table.getRowModel().rows.map((row) => ( | ||
| <tr key={row.id}> | ||
| {row.getVisibleCells().map((cell) => ( | ||
| <td key={cell.id} className="!px-0"> | ||
| {flexRender(cell.column.columnDef.cell, cell.getContext())} | ||
| </td> | ||
| ))} | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Validate rendering logic and error handling in OrderHistory.
The OrderHistory component correctly handles the loading state and uses useReactTable for rendering table data. However, there is no visible error handling or feedback for cases where data fetching might fail. Providing feedback to the user in case of errors would enhance the user experience.
+ if (error) {
+ return <div>Error loading orders. Please try again later.</div>;
+ }Committable suggestion was skipped due to low confidence.
| export const queryOrderbookMakerFee = createNodeQuery< | ||
| OrderbookMakerFeeResponse, | ||
| { | ||
| orderbookAddress: string; | ||
| } | ||
| >({ | ||
| path: ({ orderbookAddress }: { orderbookAddress: string }) => { | ||
| const msg = JSON.stringify({ | ||
| get_maker_fee: {}, | ||
| }); | ||
| const encodedMsg = Buffer.from(msg).toString("base64"); | ||
|
|
||
| return `/cosmwasm/wasm/v1/contract/${orderbookAddress}/smart/${encodedMsg}`; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper handling and validation of inputs in queryOrderbookMakerFee.
The search results indicate multiple occurrences of orderbookAddress across various files, but none show validation for this input. It is crucial to validate the orderbookAddress to ensure it is a properly formatted address, thereby preventing potential security issues, such as injection attacks.
- Location to address:
packages/server/src/queries/osmosis/orderbooks.ts, lines 7-21
Consider adding validation logic for orderbookAddress to ensure it meets expected formatting criteria before using it in the query.
Analysis chain
Ensure proper handling and validation of inputs in queryOrderbookMakerFee.
This function constructs a query using an input address without apparent validation. Consider validating the orderbookAddress to ensure it is a properly formatted address to prevent potential security issues, such as injection attacks.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for validation of `orderbookAddress` in the entire codebase.
rg --type python $'orderbookAddress'
Length of output: 66
Script:
#!/bin/bash
# Description: Check for validation of `orderbookAddress` in the entire codebase.
rg 'orderbookAddress'
Length of output: 5429
| export const queryOrderbookTicks = createNodeQuery< | ||
| OrderbookTicksResponse, | ||
| { | ||
| orderbookAddress: string; | ||
| tickIds: number[]; | ||
| } | ||
| >({ | ||
| path: ({ tickIds, orderbookAddress }) => { | ||
| const msg = JSON.stringify({ | ||
| ticks_by_id: { | ||
| tick_ids: tickIds, | ||
| }, | ||
| }); | ||
| const encodedMsg = Buffer.from(msg).toString("base64"); | ||
| return `/cosmwasm/wasm/v1/contract/${orderbookAddress}/smart/${encodedMsg}`; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Tip
Codebase Verification
Add input validation for queryOrderbookTicks.
The function constructs a query using potentially unvalidated tickIds and orderbookAddress. There is no evidence of input validation for these parameters within the file. Ensure tickIds are integers and orderbookAddress is a valid address.
packages/server/src/queries/osmosis/orderbooks.ts: Add validation fortickIdsandorderbookAddress.
Analysis chain
Add input validation for queryOrderbookTicks.
This function constructs a query using potentially unvalidated tick IDs and an address. Validate tickIds to ensure they are integers and orderbookAddress to ensure it is a valid address.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for validation of `tickIds` and `orderbookAddress` in the entire codebase.
rg --type python $'tickIds|orderbookAddress'
Length of output: 76
Script:
#!/bin/bash
# Description: Check for validation of `tickIds` and `orderbookAddress` in the entire codebase.
rg --type js 'tickIds|orderbookAddress'
Length of output: 41
Script:
#!/bin/bash
# Description: Check for validation of `tickIds` and `orderbookAddress` within the specific file and its imports.
rg 'tickIds|orderbookAddress' packages/server/src/queries/osmosis/orderbooks.ts
Length of output: 1010
| <button | ||
| onClick={() => setType(id)} | ||
| className={classNames( | ||
| "flex gap-3 rounded-lg py-2 px-3 transition-colors", | ||
| { "bg-osmoverse-700": active || isSelected } | ||
| )} | ||
| > |
There was a problem hiding this comment.
Specify the button type to avoid unexpected form submissions.
It is a best practice to explicitly set the type of buttons to prevent them from unintentionally submitting forms if they are nested within one. The default type for a button in a form is 'submit'.
- <button
+ <button type="button"
onClick={() => setType(id)}
className={classNames(
"flex gap-3 rounded-lg py-2 px-3 transition-colors",
{ "bg-osmoverse-700": active || isSelected }
)}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.
| <button | |
| onClick={() => setType(id)} | |
| className={classNames( | |
| "flex gap-3 rounded-lg py-2 px-3 transition-colors", | |
| { "bg-osmoverse-700": active || isSelected } | |
| )} | |
| > | |
| <button | |
| type="button" | |
| onClick={() => setType(id)} | |
| className={classNames( | |
| "flex gap-3 rounded-lg py-2 px-3 transition-colors", | |
| { "bg-osmoverse-700": active || isSelected } | |
| )} | |
| > |
Tools
Biome
[error] 83-89: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
| <div | ||
| className={classNames( | ||
| "absolute flex w-full items-center justify-center text-h3 transition-all", | ||
| { | ||
| [nonFocusedClasses]: !isFocused, | ||
| [focusedClasses]: isFocused, | ||
| "text-rust-300": isFocused && insufficentFunds, | ||
| "text-wosmongton-400": isFocused && (amount === "0" || amount === ""), | ||
| "text-white-full": isFocused && +amount > 0, | ||
| } | ||
| )} | ||
| onClick={ | ||
| !disableSwitching && focused === oppositeTypeEnum | ||
| ? swapFocus | ||
| : undefined | ||
| } | ||
| > |
There was a problem hiding this comment.
Ensure accessibility by pairing click events with keyboard events.
The current implementation of the click event does not include corresponding keyboard events. This is an accessibility issue as actions triggered using mouse events should also be accessible via keyboard events to accommodate keyboard-only users.
187a188,189
+ onKeyUp={focused === oppositeTypeEnum ? swapFocus : undefined}
+ onKeyDown={focused === oppositeTypeEnum ? swapFocus : undefined}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.
| <div | |
| className={classNames( | |
| "absolute flex w-full items-center justify-center text-h3 transition-all", | |
| { | |
| [nonFocusedClasses]: !isFocused, | |
| [focusedClasses]: isFocused, | |
| "text-rust-300": isFocused && insufficentFunds, | |
| "text-wosmongton-400": isFocused && (amount === "0" || amount === ""), | |
| "text-white-full": isFocused && +amount > 0, | |
| } | |
| )} | |
| onClick={ | |
| !disableSwitching && focused === oppositeTypeEnum | |
| ? swapFocus | |
| : undefined | |
| } | |
| > | |
| <div | |
| className={classNames( | |
| "absolute flex w-full items-center justify-center text-h3 transition-all", | |
| { | |
| [nonFocusedClasses]: !isFocused, | |
| [focusedClasses]: isFocused, | |
| "text-rust-300": isFocused && insufficentFunds, | |
| "text-wosmongton-400": isFocused && (amount === "0" || amount === ""), | |
| "text-white-full": isFocused && +amount > 0, | |
| } | |
| )} | |
| onClick={ | |
| !disableSwitching && focused === oppositeTypeEnum | |
| ? swapFocus | |
| : undefined | |
| } | |
| onKeyUp={focused === oppositeTypeEnum ? swapFocus : undefined} | |
| onKeyDown={focused === oppositeTypeEnum ? swapFocus : undefined} | |
| > |
Tools
Biome
[error] 171-187: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
There was a problem hiding this comment.
Actionable comments posted: 18
Outside diff range and nitpick comments (1)
packages/web/pages/index.tsx (1)
Line range hint
93-99: Reiterate: Alternative text title element cannot be emptyFor accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
+ aria-label="Decorative background"
| /> | ||
| </div> | ||
| <input | ||
| autoFocus |
There was a problem hiding this comment.
Consider accessibility improvements by removing autoFocus.
The autoFocus attribute can disrupt screen reader users by changing the focus context unexpectedly. Consider removing it or managing focus more contextually.
279c279
< autoFocus
---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.
| autoFocus |
Tools
Biome
[error] 279-279: Avoid the autoFocus attribute.
Unsafe fix: Remove the autoFocus attribute.
(lint/a11y/noAutofocus)
| )} | ||
| <div className="flex flex-col"> | ||
| {showSearchBox && ( | ||
| <div className="px-8" onClick={(e) => e.stopPropagation()}> |
There was a problem hiding this comment.
Enhance keyboard accessibility.
The onClick event handler should be complemented with keyboard event handlers to ensure accessibility for users relying on keyboards.
270a271,273
> onKeyUp={(e) => {
> if (e.key === "Enter") onSearch(e.target.value);
> }}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.
| <div className="px-8" onClick={(e) => e.stopPropagation()}> | |
| <div className="px-8" onClick={(e) => e.stopPropagation()} onKeyUp={(e) => { | |
| if (e.key === "Enter") onSearch(e.target.value); | |
| }}> |
Tools
Biome
[error] 270-270: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
| <button | ||
| onClick={onClose} | ||
| className="absolute right-4 flex h-12 w-12 items-center justify-center rounded-full bg-osmoverse-800" | ||
| > |
There was a problem hiding this comment.
Ensure button type is explicitly set.
The button does not specify the type attribute. It's crucial to set this to button to prevent unintended form submissions if this component is nested inside a form.
239a240
> type="button"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.
| <button | |
| onClick={onClose} | |
| className="absolute right-4 flex h-12 w-12 items-center justify-center rounded-full bg-osmoverse-800" | |
| > | |
| <button | |
| type="button" | |
| onClick={onClose} | |
| className="absolute right-4 flex h-12 w-12 items-center justify-center rounded-full bg-osmoverse-800" | |
| > |
Tools
Biome
[error] 239-242: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| <button | ||
| key={`swap-tab-${tab.value}`} | ||
| onClick={() => setTab(tab.value)} | ||
| className={classNames("rounded-3xl px-4 py-3", { | ||
| "bg-osmoverse-700": isActive, | ||
| })} | ||
| > |
There was a problem hiding this comment.
Reiterate: Add explicit type attribute to button elements to prevent unintended form submissions.
As previously noted, the button elements in the SwapToolTabs component are missing an explicit type attribute. This can lead to unintended form submissions if the component is used within a form. It's crucial to specify this to avoid potential issues in single-page applications.
+ type="button"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.
| <button | |
| key={`swap-tab-${tab.value}`} | |
| onClick={() => setTab(tab.value)} | |
| className={classNames("rounded-3xl px-4 py-3", { | |
| "bg-osmoverse-700": isActive, | |
| })} | |
| > | |
| <button | |
| key={`swap-tab-${tab.value}`} | |
| onClick={() => setTab(tab.value)} | |
| className={classNames("rounded-3xl px-4 py-3", { | |
| "bg-osmoverse-700": isActive, | |
| })} | |
| type="button" | |
| > |
Tools
Biome
[error] 53-59: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| <button | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| if (tokenSelectionAvailable) { | ||
| setIsSelectOpen(!isSelectOpen); | ||
| } | ||
| }} | ||
| className="flex items-center justify-between rounded-t-xl bg-osmoverse-850 py-3 px-5 md:justify-start" | ||
| > |
There was a problem hiding this comment.
Specify button type to avoid form submission issues.
The button element within the component should explicitly specify its type to prevent unintended form submissions when nested inside form elements. The default type for a button in HTML is "submit", which can cause issues in single-page applications like React.
- <button onClick={(e) => {
+ <button type="button" onClick={(e) => {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.
| <button | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| if (tokenSelectionAvailable) { | |
| setIsSelectOpen(!isSelectOpen); | |
| } | |
| }} | |
| className="flex items-center justify-between rounded-t-xl bg-osmoverse-850 py-3 px-5 md:justify-start" | |
| > | |
| <button | |
| type="button" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| if (tokenSelectionAvailable) { | |
| setIsSelectOpen(!isSelectOpen); | |
| } | |
| }} | |
| className="flex items-center justify-between rounded-t-xl bg-osmoverse-850 py-3 px-5 md:justify-start" | |
| > |
Tools
Biome
[error] 114-122: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
packages/web/modals/review-swap.tsx
Outdated
| </span> | ||
| } | ||
| /> | ||
| <RecapRow left={t("limitOrders.receiveMin")} right={<></>} /> |
There was a problem hiding this comment.
Remove unnecessary React Fragment.
The usage of React Fragment here is redundant since it wraps a single child. Removing it can simplify the markup without affecting functionality.
- <RecapRow left="Recieve minimum" right={<></>} />
+ <RecapRow left="Recieve minimum" right="" />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.
| <RecapRow left={t("limitOrders.receiveMin")} right={<></>} /> | |
| <RecapRow left={t("limitOrders.receiveMin")} right="" /> |
Tools
Biome
[error] 166-166: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
(lint/complexity/noUselessFragments)
packages/web/modals/review-swap.tsx
Outdated
| <button | ||
| onClick={onClose} | ||
| className="absolute right-4 flex h-12 w-12 items-center justify-center rounded-full bg-osmoverse-800" | ||
| > |
There was a problem hiding this comment.
Specify button type to avoid form submission issues.
Ensure that all button elements within the modal explicitly declare their type to prevent default form submission behavior.
- <button
+ <button type="button"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.
| <button | |
| onClick={onClose} | |
| className="absolute right-4 flex h-12 w-12 items-center justify-center rounded-full bg-osmoverse-800" | |
| > | |
| <button | |
| type="button" | |
| onClick={onClose} | |
| className="absolute right-4 flex h-12 w-12 items-center justify-center rounded-full bg-osmoverse-800" | |
| > |
Tools
Biome
[error] 42-45: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| <div | ||
| className="flex w-full cursor-pointer items-center justify-between" | ||
| onClick={() => setDisplayInfo(!displayInfo && !isLoading)} | ||
| > |
There was a problem hiding this comment.
Reiterate: Improve Accessibility for Clickable Elements
As previously noted, the div element with an onClick event should also have corresponding keyboard event handlers (like onKeyUp, onKeyDown, or onKeyPress) to ensure accessibility for users who rely on keyboard navigation. Consider using a button or a link element for better semantic HTML and built-in keyboard accessibility.
+ role="button"
+ tabIndex="0"
+ onKeyUp={(event) => { if (event.key === 'Enter') setDisplayInfo(!displayInfo && !isLoading); }}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.
| <div | |
| className="flex w-full cursor-pointer items-center justify-between" | |
| onClick={() => setDisplayInfo(!displayInfo && !isLoading)} | |
| > | |
| <div | |
| className="flex w-full cursor-pointer items-center justify-between" | |
| onClick={() => setDisplayInfo(!displayInfo && !isLoading)} | |
| role="button" | |
| tabIndex="0" | |
| onKeyUp={(event) => { if (event.key === 'Enter') setDisplayInfo(!displayInfo && !isLoading); }} | |
| > |
Tools
Biome
[error] 43-46: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
| const [tab, setTab] = useQueryState( | ||
| "tab", | ||
| parseAsStringLiteral(TX_PAGE_TABS).withDefault("history") | ||
| ); |
There was a problem hiding this comment.
Reiterate: Consider using optional chaining for better safety and readability.
As previously noted, the static analysis tool suggests using optional chaining here to ensure that the code does not throw an error if any part of the chain is undefined. This is especially important in JavaScript where accessing properties on undefined or null will throw a runtime error.
+ wallet?.isWalletConnected && wallet?.address && transactions?.length > 0;Committable suggestion was skipped due to low confidence.
| <hr className="my-2 text-osmoverse-700" /> | ||
| <RecapRow | ||
| left={t("limitOrders.receive")} | ||
| right={<>{formatPretty(total)}</>} |
There was a problem hiding this comment.
Remove unnecessary React Fragment.
The usage of React Fragment here is redundant since it wraps a single child. Removing it can simplify the markup without affecting functionality.
- <RecapRow left="Recieve" right={<>{formatPretty(total)}</>} />
+ <RecapRow left="Recieve" right={formatPretty(total)} />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.
| right={<>{formatPretty(total)}</>} | |
| right={formatPretty(total)} |
Tools
Biome
[error] 135-135: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
| return ( | ||
| <Menu as="div" className="relative inline-block"> | ||
| <Menu.Button className="flex items-center gap-2 rounded-[48px] bg-osmoverse-825 py-3 px-4"> | ||
| <p className="font-semibold text-wosmongton-200"> | ||
| {type === "market" ? t("limitOrders.market") : t("limitOrders.limit")} | ||
| </p> | ||
| <div className="flex h-6 w-6 items-center justify-center"> | ||
| <Icon id="chevron-down" className="h-[7px] w-3 text-wosmongton-200" /> | ||
| </div> | ||
| </Menu.Button> | ||
| <Transition | ||
| as={Fragment} | ||
| enter="transition ease-out duration-100" | ||
| enterFrom="transform opacity-0 scale-95" | ||
| enterTo="transform opacity-100 scale-100" | ||
| leave="transition ease-in duration-75" | ||
| leaveFrom="transform opacity-100 scale-100" | ||
| leaveTo="transform opacity-0 scale-95" | ||
| > | ||
| <Menu.Items className="absolute right-0 z-50 mt-3 flex w-[280px] origin-top-right flex-col rounded-xl bg-osmoverse-800"> | ||
| <div className="flex items-center border-b border-osmoverse-700 py-2 px-4"> | ||
| <p className="text-subtitle1 font-semibold"> | ||
| {t("limitOrders.orderType")} | ||
| </p> | ||
| </div> | ||
| <div className="flex flex-col gap-2 p-2"> | ||
| {uiTradeTypes.map(({ id, title, description, icon }) => { | ||
| const isSelected = type === id; | ||
|
|
||
| return ( | ||
| <Menu.Item key={title}> | ||
| {({ active }) => ( | ||
| <button | ||
| onClick={() => setType(id)} | ||
| className={classNames( | ||
| "flex gap-3 rounded-lg py-2 px-3 transition-colors", | ||
| { "bg-osmoverse-700": active || isSelected } | ||
| )} | ||
| > | ||
| <div className="flex h-6 w-6 items-center justify-center"> | ||
| <Icon | ||
| id={icon} | ||
| className={classNames( | ||
| "h-6 w-6 text-osmoverse-400 transition-colors", | ||
| { | ||
| "text-white-full": active || isSelected, | ||
| } | ||
| )} | ||
| /> | ||
| </div> | ||
| <div className="flex flex-col gap-1 text-left"> | ||
| <p>{title}</p> | ||
| <small className="text-sm leading-5 text-osmoverse-300"> | ||
| {description} | ||
| </small> | ||
| </div> | ||
| </button> | ||
| )} | ||
| </Menu.Item> | ||
| ); | ||
| })} | ||
| </div> | ||
| </Menu.Items> | ||
| </Transition> | ||
| </Menu> | ||
| ); |
There was a problem hiding this comment.
Specify the button type to avoid unexpected form submissions.
It is a best practice to explicitly set the type of buttons to prevent them from unintentionally submitting forms if they are nested within one. The default type for a button in a form is 'submit'.
- <button
+ <button type="button"
onClick={() => setType(id)}
className={classNames(
"flex gap-3 rounded-lg py-2 px-3 transition-colors",
{ "bg-osmoverse-700": active || isSelected }
)}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.
| return ( | |
| <Menu as="div" className="relative inline-block"> | |
| <Menu.Button className="flex items-center gap-2 rounded-[48px] bg-osmoverse-825 py-3 px-4"> | |
| <p className="font-semibold text-wosmongton-200"> | |
| {type === "market" ? t("limitOrders.market") : t("limitOrders.limit")} | |
| </p> | |
| <div className="flex h-6 w-6 items-center justify-center"> | |
| <Icon id="chevron-down" className="h-[7px] w-3 text-wosmongton-200" /> | |
| </div> | |
| </Menu.Button> | |
| <Transition | |
| as={Fragment} | |
| enter="transition ease-out duration-100" | |
| enterFrom="transform opacity-0 scale-95" | |
| enterTo="transform opacity-100 scale-100" | |
| leave="transition ease-in duration-75" | |
| leaveFrom="transform opacity-100 scale-100" | |
| leaveTo="transform opacity-0 scale-95" | |
| > | |
| <Menu.Items className="absolute right-0 z-50 mt-3 flex w-[280px] origin-top-right flex-col rounded-xl bg-osmoverse-800"> | |
| <div className="flex items-center border-b border-osmoverse-700 py-2 px-4"> | |
| <p className="text-subtitle1 font-semibold"> | |
| {t("limitOrders.orderType")} | |
| </p> | |
| </div> | |
| <div className="flex flex-col gap-2 p-2"> | |
| {uiTradeTypes.map(({ id, title, description, icon }) => { | |
| const isSelected = type === id; | |
| return ( | |
| <Menu.Item key={title}> | |
| {({ active }) => ( | |
| <button | |
| onClick={() => setType(id)} | |
| className={classNames( | |
| "flex gap-3 rounded-lg py-2 px-3 transition-colors", | |
| { "bg-osmoverse-700": active || isSelected } | |
| )} | |
| > | |
| <div className="flex h-6 w-6 items-center justify-center"> | |
| <Icon | |
| id={icon} | |
| className={classNames( | |
| "h-6 w-6 text-osmoverse-400 transition-colors", | |
| { | |
| "text-white-full": active || isSelected, | |
| } | |
| )} | |
| /> | |
| </div> | |
| <div className="flex flex-col gap-1 text-left"> | |
| <p>{title}</p> | |
| <small className="text-sm leading-5 text-osmoverse-300"> | |
| {description} | |
| </small> | |
| </div> | |
| </button> | |
| )} | |
| </Menu.Item> | |
| ); | |
| })} | |
| </div> | |
| </Menu.Items> | |
| </Transition> | |
| </Menu> | |
| ); | |
| return ( | |
| <Menu as="div" className="relative inline-block"> | |
| <Menu.Button className="flex items-center gap-2 rounded-[48px] bg-osmoverse-825 py-3 px-4"> | |
| <p className="font-semibold text-wosmongton-200"> | |
| {type === "market" ? t("limitOrders.market") : t("limitOrders.limit")} | |
| </p> | |
| <div className="flex h-6 w-6 items-center justify-center"> | |
| <Icon id="chevron-down" className="h-[7px] w-3 text-wosmongton-200" /> | |
| </div> | |
| </Menu.Button> | |
| <Transition | |
| as={Fragment} | |
| enter="transition ease-out duration-100" | |
| enterFrom="transform opacity-0 scale-95" | |
| enterTo="transform opacity-100 scale-100" | |
| leave="transition ease-in duration-75" | |
| leaveFrom="transform opacity-100 scale-100" | |
| leaveTo="transform opacity-0 scale-95" | |
| > | |
| <Menu.Items className="absolute right-0 z-50 mt-3 flex w-[280px] origin-top-right flex-col rounded-xl bg-osmoverse-800"> | |
| <div className="flex items-center border-b border-osmoverse-700 py-2 px-4"> | |
| <p className="text-subtitle1 font-semibold"> | |
| {t("limitOrders.orderType")} | |
| </p> | |
| </div> | |
| <div className="flex flex-col gap-2 p-2"> | |
| {uiTradeTypes.map(({ id, title, description, icon }) => { | |
| const isSelected = type === id; | |
| return ( | |
| <Menu.Item key={title}> | |
| {({ active }) => ( | |
| <button type="button" | |
| onClick={() => setType(id)} | |
| className={classNames( | |
| "flex gap-3 rounded-lg py-2 px-3 transition-colors", | |
| { "bg-osmoverse-700": active || isSelected } | |
| )} | |
| > | |
| <div className="flex h-6 w-6 items-center justify-center"> | |
| <Icon | |
| id={icon} | |
| className={classNames( | |
| "h-6 w-6 text-osmoverse-400 transition-colors", | |
| { | |
| "text-white-full": active || isSelected, | |
| } | |
| )} | |
| /> | |
| </div> | |
| <div className="flex flex-col gap-1 text-left"> | |
| <p>{title}</p> | |
| <small className="text-sm leading-5 text-osmoverse-300"> | |
| {description} | |
| </small> | |
| </div> | |
| </button> | |
| )} | |
| </Menu.Item> | |
| ); | |
| })} | |
| </div> | |
| </Menu.Items> | |
| </Transition> | |
| </Menu> | |
| ); |
Tools
Biome
[error] 93-99: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| // Hacky solution to deal with rounding | ||
| // TODO: Investigate a way to improve this |
There was a problem hiding this comment.
Reminder: Improve hacky solution for rounding.
The TODO comment indicates that the current solution for rounding is hacky and needs improvement.
Do you want me to investigate and propose a more robust solution for this rounding issue or open a GitHub issue to track this task?
| if (from === quote) { | ||
| if (quote === "USDC") { | ||
| set({ quote: "USDT" }); | ||
| } else { | ||
| set({ quote: "USDC" }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Simplify conditional logic.
The nested if-else structure can be simplified for better readability.
if (from === quote) {
set({ quote: quote === "USDC" ? "USDT" : "USDC" });
}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 (from === quote) { | |
| if (quote === "USDC") { | |
| set({ quote: "USDT" }); | |
| } else { | |
| set({ quote: "USDC" }); | |
| } | |
| } | |
| if (from === quote) { | |
| set({ quote: quote === "USDC" ? "USDT" : "USDC" }); | |
| } |
| export const PlaceLimitTool: FunctionComponent<PlaceLimitToolProps> = observer( | ||
| ({ page, refetchOrders }: PlaceLimitToolProps) => { |
There was a problem hiding this comment.
Consider replacing the interface with a type alias.
The interface PlaceLimitToolProps is simple and could be replaced with a type alias for better clarity.
- export interface PlaceLimitToolProps {
- page: EventPage;
- refetchOrders: () => Promise<void>;
- }
+ export type PlaceLimitToolProps = {
+ page: EventPage;
+ refetchOrders: () => Promise<void>;
+ };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.
| export const PlaceLimitTool: FunctionComponent<PlaceLimitToolProps> = observer( | |
| ({ page, refetchOrders }: PlaceLimitToolProps) => { | |
| export type PlaceLimitToolProps = { | |
| page: EventPage; | |
| refetchOrders: () => Promise<void>; | |
| }; | |
| export const PlaceLimitTool: FunctionComponent<PlaceLimitToolProps> = observer( | |
| ({ page, refetchOrders }: PlaceLimitToolProps) => { |
| const isMarketLoading = useMemo(() => { | ||
| return ( | ||
| swapState.isMarket && | ||
| (swapState.marketState.isQuoteLoading || | ||
| Boolean(swapState.marketState.isLoadingNetworkFee)) && | ||
| !Boolean(swapState.marketState.error) |
There was a problem hiding this comment.
Remove redundant Boolean call.
The Boolean constructor is redundant since the value will already be coerced to a boolean.
return (
swapState.isMarket &&
(swapState.marketState.isQuoteLoading ||
swapState.marketState.isLoadingNetworkFee) &&
!swapState.marketState.error
);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 isMarketLoading = useMemo(() => { | |
| return ( | |
| swapState.isMarket && | |
| (swapState.marketState.isQuoteLoading || | |
| Boolean(swapState.marketState.isLoadingNetworkFee)) && | |
| !Boolean(swapState.marketState.error) | |
| const isMarketLoading = useMemo(() => { | |
| return ( | |
| swapState.isMarket && | |
| (swapState.marketState.isQuoteLoading || | |
| swapState.marketState.isLoadingNetworkFee) && | |
| !swapState.marketState.error | |
| ); |
Tools
Biome
[error] 180-180: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
| const hasFunds = useMemo( | ||
| () => | ||
| (tab === "buy" | ||
| ? swapState.quoteTokenBalance | ||
| : swapState.baseTokenBalance | ||
| ) | ||
| ?.toDec() | ||
| .gt(new Dec(0)), | ||
| [swapState.baseTokenBalance, swapState.quoteTokenBalance, tab] | ||
| ); |
There was a problem hiding this comment.
Optimize hasFunds calculation.
The hasFunds calculation can be simplified for better readability.
const hasFunds = useMemo(
() => {
const balance = tab === "buy"
? swapState.quoteTokenBalance
: swapState.baseTokenBalance;
return balance?.toDec().gt(new Dec(0));
},
[swapState.baseTokenBalance, swapState.quoteTokenBalance, tab]
);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 hasFunds = useMemo( | |
| () => | |
| (tab === "buy" | |
| ? swapState.quoteTokenBalance | |
| : swapState.baseTokenBalance | |
| ) | |
| ?.toDec() | |
| .gt(new Dec(0)), | |
| [swapState.baseTokenBalance, swapState.quoteTokenBalance, tab] | |
| ); | |
| const hasFunds = useMemo( | |
| () => { | |
| const balance = tab === "buy" | |
| ? swapState.quoteTokenBalance | |
| : swapState.baseTokenBalance; | |
| return balance?.toDec().gt(new Dec(0)); | |
| }, | |
| [swapState.baseTokenBalance, swapState.quoteTokenBalance, tab] | |
| ); |
| const error = useMemo(() => { | ||
| if ( | ||
| !Boolean(orderbook) || | ||
| !Boolean(orderbook!.poolId) || | ||
| orderbook!.poolId === "" | ||
| ) { | ||
| return "errors.noOrderbook"; | ||
| } | ||
|
|
||
| if (Boolean(makerFeeError)) { | ||
| return makerFeeError?.message; | ||
| } | ||
| }, [orderbook, makerFeeError]); |
There was a problem hiding this comment.
Add error handling in useOrderbook.
Consider adding error handling to manage potential errors during the data fetching process to improve robustness.
const error = useMemo(() => {
if (
- !Boolean(orderbook) ||
- !Boolean(orderbook!.poolId) ||
+ !orderbook ||
+ !orderbook!.poolId ||
orderbook!.poolId === ""
) {
return "errors.noOrderbook";
}
- if (Boolean(makerFeeError)) {
+ if (makerFeeError) {
return makerFeeError?.message;
}
+ if (error) {
+ console.error("Failed to fetch orderbook data:", error);
+ }
}, [orderbook, makerFeeError]);Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 212-212: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
[error] 213-213: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
[error] 219-219: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
| export const useOrderbookAllActiveOrders = ({ | ||
| userAddress, | ||
| pageSize = 10, | ||
| refetchInterval = 2000, | ||
| }: { | ||
| userAddress: string; | ||
| pageSize?: number; | ||
| refetchInterval?: number; | ||
| }) => { | ||
| const { orderbooks } = useOrderbooks(); | ||
| const addresses = orderbooks.map(({ contractAddress }) => contractAddress); | ||
| const { | ||
| data: orders, | ||
| isLoading, | ||
| fetchNextPage, | ||
| isFetching, | ||
| isFetchingNextPage, | ||
| hasNextPage, | ||
| refetch, | ||
| isRefetching, | ||
| } = api.edge.orderbooks.getAllOrders.useInfiniteQuery( | ||
| { | ||
| contractAddresses: addresses, | ||
| userOsmoAddress: userAddress, | ||
| limit: pageSize, | ||
| }, | ||
| { | ||
| getNextPageParam: (lastPage) => lastPage.nextCursor, | ||
| initialCursor: 0, | ||
| refetchInterval, | ||
| enabled: !!userAddress && addresses.length > 0, | ||
| refetchOnMount: true, | ||
| keepPreviousData: true, | ||
| } | ||
| ); | ||
|
|
||
| const allOrders = useMemo(() => { | ||
| return orders?.pages.flatMap((page) => page.items) ?? []; | ||
| }, [orders]); | ||
| return { | ||
| orders: allOrders, | ||
| isLoading, | ||
| fetchNextPage, | ||
| isFetching, | ||
| isFetchingNextPage, | ||
| hasNextPage, | ||
| refetch, | ||
| isRefetching, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Add error handling in useOrderbookAllActiveOrders.
Consider adding error handling to manage potential errors during the data fetching process to improve robustness.
const {
data: orders,
isLoading,
fetchNextPage,
isFetching,
isFetchingNextPage,
hasNextPage,
+ error,
refetch,
isRefetching,
} = api.edge.orderbooks.getAllOrders.useInfiniteQuery(
{
contractAddresses: addresses,
userOsmoAddress: userAddress,
limit: pageSize,
},
{
getNextPageParam: (lastPage) => lastPage.nextCursor,
initialCursor: 0,
refetchInterval,
enabled: !!userAddress && addresses.length > 0,
refetchOnMount: true,
keepPreviousData: true,
}
);
if (error) {
console.error("Failed to fetch all active orders:", error);
}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.
| export const useOrderbookAllActiveOrders = ({ | |
| userAddress, | |
| pageSize = 10, | |
| refetchInterval = 2000, | |
| }: { | |
| userAddress: string; | |
| pageSize?: number; | |
| refetchInterval?: number; | |
| }) => { | |
| const { orderbooks } = useOrderbooks(); | |
| const addresses = orderbooks.map(({ contractAddress }) => contractAddress); | |
| const { | |
| data: orders, | |
| isLoading, | |
| fetchNextPage, | |
| isFetching, | |
| isFetchingNextPage, | |
| hasNextPage, | |
| refetch, | |
| isRefetching, | |
| } = api.edge.orderbooks.getAllOrders.useInfiniteQuery( | |
| { | |
| contractAddresses: addresses, | |
| userOsmoAddress: userAddress, | |
| limit: pageSize, | |
| }, | |
| { | |
| getNextPageParam: (lastPage) => lastPage.nextCursor, | |
| initialCursor: 0, | |
| refetchInterval, | |
| enabled: !!userAddress && addresses.length > 0, | |
| refetchOnMount: true, | |
| keepPreviousData: true, | |
| } | |
| ); | |
| const allOrders = useMemo(() => { | |
| return orders?.pages.flatMap((page) => page.items) ?? []; | |
| }, [orders]); | |
| return { | |
| orders: allOrders, | |
| isLoading, | |
| fetchNextPage, | |
| isFetching, | |
| isFetchingNextPage, | |
| hasNextPage, | |
| refetch, | |
| isRefetching, | |
| }; | |
| }; | |
| export const useOrderbookAllActiveOrders = ({ | |
| userAddress, | |
| pageSize = 10, | |
| refetchInterval = 2000, | |
| }: { | |
| userAddress: string; | |
| pageSize?: number; | |
| refetchInterval?: number; | |
| }) => { | |
| const { orderbooks } = useOrderbooks(); | |
| const addresses = orderbooks.map(({ contractAddress }) => contractAddress); | |
| const { | |
| data: orders, | |
| isLoading, | |
| fetchNextPage, | |
| isFetching, | |
| isFetchingNextPage, | |
| hasNextPage, | |
| error, | |
| refetch, | |
| isRefetching, | |
| } = api.edge.orderbooks.getAllOrders.useInfiniteQuery( | |
| { | |
| contractAddresses: addresses, | |
| userOsmoAddress: userAddress, | |
| limit: pageSize, | |
| }, | |
| { | |
| getNextPageParam: (lastPage) => lastPage.nextCursor, | |
| initialCursor: 0, | |
| refetchInterval, | |
| enabled: !!userAddress && addresses.length > 0, | |
| refetchOnMount: true, | |
| keepPreviousData: true, | |
| } | |
| ); | |
| if (error) { | |
| console.error("Failed to fetch all active orders:", error); | |
| } | |
| const allOrders = useMemo(() => { | |
| return orders?.pages.flatMap((page) => page.items) ?? []; | |
| }, [orders]); | |
| return { | |
| orders: allOrders, | |
| isLoading, | |
| fetchNextPage, | |
| isFetching, | |
| isFetchingNextPage, | |
| hasNextPage, | |
| refetch, | |
| isRefetching, | |
| }; | |
| }; |
| export const useOrderbookClaimableOrders = ({ | ||
| userAddress, | ||
| }: { | ||
| userAddress: string; | ||
| }) => { | ||
| const { orderbooks } = useOrderbooks(); | ||
| const { accountStore } = useStore(); | ||
| const account = accountStore.getWallet(accountStore.osmosisChainId); | ||
| const addresses = orderbooks.map(({ contractAddress }) => contractAddress); | ||
| const { | ||
| data: orders, | ||
| isLoading, | ||
| isFetching, | ||
| refetch, | ||
| } = api.edge.orderbooks.getClaimableOrders.useQuery( | ||
| { | ||
| contractAddresses: addresses, | ||
| userOsmoAddress: userAddress, | ||
| }, | ||
| { | ||
| enabled: !!userAddress && addresses.length > 0, | ||
| refetchInterval: 5000, | ||
| refetchOnMount: true, | ||
| } | ||
| ); | ||
|
|
||
| const claimAllOrders = useCallback(async () => { | ||
| if (!account || !orders) return; | ||
| const msgs = addresses | ||
| .map((contractAddress) => { | ||
| const ordersForAddress = orders.filter( | ||
| (o) => o.orderbookAddress === contractAddress | ||
| ); | ||
| if (ordersForAddress.length === 0) return; | ||
|
|
||
| const msg = { | ||
| batch_claim: { | ||
| orders: ordersForAddress.map((o) => [o.tick_id, o.order_id]), | ||
| }, | ||
| }; | ||
| return { | ||
| contractAddress, | ||
| msg, | ||
| funds: [], | ||
| }; | ||
| }) | ||
| .filter(Boolean) as { | ||
| contractAddress: string; | ||
| msg: object; | ||
| funds: CoinPrimitive[]; | ||
| }[]; | ||
|
|
||
| if (msgs.length > 0) { | ||
| await account?.cosmwasm.sendMultiExecuteContractMsg("executeWasm", msgs); | ||
| await refetch(); | ||
| } | ||
| }, [orders, account, addresses, refetch]); | ||
|
|
||
| return { | ||
| orders: orders ?? [], | ||
| count: orders?.length ?? 0, | ||
| isLoading: isLoading || isFetching, | ||
| claimAllOrders, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Add error handling in useOrderbookClaimableOrders.
Consider adding error handling to manage potential errors during the data fetching process to improve robustness.
const {
data: orders,
isLoading,
isFetching,
+ error,
refetch,
} = api.edge.orderbooks.getClaimableOrders.useQuery(
{
contractAddresses: addresses,
userOsmoAddress: userAddress,
},
{
enabled: !!userAddress && addresses.length > 0,
refetchInterval: 5000,
refetchOnMount: true,
}
);
if (error) {
console.error("Failed to fetch claimable orders:", error);
}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.
| export const useOrderbookClaimableOrders = ({ | |
| userAddress, | |
| }: { | |
| userAddress: string; | |
| }) => { | |
| const { orderbooks } = useOrderbooks(); | |
| const { accountStore } = useStore(); | |
| const account = accountStore.getWallet(accountStore.osmosisChainId); | |
| const addresses = orderbooks.map(({ contractAddress }) => contractAddress); | |
| const { | |
| data: orders, | |
| isLoading, | |
| isFetching, | |
| refetch, | |
| } = api.edge.orderbooks.getClaimableOrders.useQuery( | |
| { | |
| contractAddresses: addresses, | |
| userOsmoAddress: userAddress, | |
| }, | |
| { | |
| enabled: !!userAddress && addresses.length > 0, | |
| refetchInterval: 5000, | |
| refetchOnMount: true, | |
| } | |
| ); | |
| const claimAllOrders = useCallback(async () => { | |
| if (!account || !orders) return; | |
| const msgs = addresses | |
| .map((contractAddress) => { | |
| const ordersForAddress = orders.filter( | |
| (o) => o.orderbookAddress === contractAddress | |
| ); | |
| if (ordersForAddress.length === 0) return; | |
| const msg = { | |
| batch_claim: { | |
| orders: ordersForAddress.map((o) => [o.tick_id, o.order_id]), | |
| }, | |
| }; | |
| return { | |
| contractAddress, | |
| msg, | |
| funds: [], | |
| }; | |
| }) | |
| .filter(Boolean) as { | |
| contractAddress: string; | |
| msg: object; | |
| funds: CoinPrimitive[]; | |
| }[]; | |
| if (msgs.length > 0) { | |
| await account?.cosmwasm.sendMultiExecuteContractMsg("executeWasm", msgs); | |
| await refetch(); | |
| } | |
| }, [orders, account, addresses, refetch]); | |
| return { | |
| orders: orders ?? [], | |
| count: orders?.length ?? 0, | |
| isLoading: isLoading || isFetching, | |
| claimAllOrders, | |
| }; | |
| }; | |
| export const useOrderbookClaimableOrders = ({ | |
| userAddress, | |
| }: { | |
| userAddress: string; | |
| }) => { | |
| const { orderbooks } = useOrderbooks(); | |
| const { accountStore } = useStore(); | |
| const account = accountStore.getWallet(accountStore.osmosisChainId); | |
| const addresses = orderbooks.map(({ contractAddress }) => contractAddress); | |
| const { | |
| data: orders, | |
| isLoading, | |
| isFetching, | |
| error, | |
| refetch, | |
| } = api.edge.orderbooks.getClaimableOrders.useQuery( | |
| { | |
| contractAddresses: addresses, | |
| userOsmoAddress: userAddress, | |
| }, | |
| { | |
| enabled: !!userAddress && addresses.length > 0, | |
| refetchInterval: 5000, | |
| refetchOnMount: true, | |
| } | |
| ); | |
| if (error) { | |
| console.error("Failed to fetch claimable orders:", error); | |
| } | |
| const claimAllOrders = useCallback(async () => { | |
| if (!account || !orders) return; | |
| const msgs = addresses | |
| .map((contractAddress) => { | |
| const ordersForAddress = orders.filter( | |
| (o) => o.orderbookAddress === contractAddress | |
| ); | |
| if (ordersForAddress.length === 0) return; | |
| const msg = { | |
| batch_claim: { | |
| orders: ordersForAddress.map((o) => [o.tick_id, o.order_id]), | |
| }, | |
| }; | |
| return { | |
| contractAddress, | |
| msg, | |
| funds: [], | |
| }; | |
| }) | |
| .filter(Boolean) as { | |
| contractAddress: string; | |
| msg: object; | |
| funds: CoinPrimitive[]; | |
| }[]; | |
| if (msgs.length > 0) { | |
| await account?.cosmwasm.sendMultiExecuteContractMsg("executeWasm", msgs); | |
| await refetch(); | |
| } | |
| }, [orders, account, addresses, refetch]); | |
| return { | |
| orders: orders ?? [], | |
| count: orders?.length ?? 0, | |
| isLoading: isLoading || isFetching, | |
| claimAllOrders, | |
| }; | |
| }; |
| // const { claimAllOrders } = useOrderbookClaimableOrders({ | ||
| // userAddress: wallet?.address ?? "", | ||
| // }); | ||
| const claimAllOrders = useCallback(() => { | ||
| console.log("claimAllOrders"); | ||
| }, []); | ||
|
|
||
| const tableData: OrderCellData[] = useMemo(() => { | ||
| return orders.map((o) => ({ | ||
| ...o, | ||
| isRefetching, | ||
| refetch: async () => refetch({ stale: false }), | ||
| })); | ||
| }, [orders, isRefetching, refetch]); |
There was a problem hiding this comment.
Implement the claimAllOrders function.
The claimAllOrders function is currently a placeholder. Implement this function to allow users to claim all orders.
| export const OrderHistory = observer(() => { | ||
| const { logEvent } = useAmplitudeAnalytics({ | ||
| onLoadEvent: [EventName.LimitOrder.pageViewed], | ||
| }); | ||
| const { accountStore } = useStore(); | ||
| const { t } = useTranslation(); | ||
| const wallet = accountStore.getWallet(accountStore.osmosisChainId); | ||
|
|
||
| const { | ||
| orders, | ||
| isLoading, | ||
| fetchNextPage, | ||
| isFetchingNextPage, | ||
| hasNextPage, | ||
| refetch, | ||
| isRefetching, | ||
| } = useOrderbookAllActiveOrders({ | ||
| userAddress: wallet?.address ?? "", | ||
| pageSize: 10, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Add error handling for data fetching failures.
The OrderHistory component correctly handles the loading state and uses useOrderbookAllActiveOrders for fetching orders. However, there is no visible error handling or feedback for cases where data fetching might fail. Providing feedback to the user in case of errors would enhance the user experience.
if (isLoading) {
return (
<div className="flex items-center justify-center py-10">
<Spinner className="!h-10 !w-10" />
</div>
);
}
+ if (error) {
+ return <div>Error loading orders. Please try again later.</div>;
+ }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.
| export const OrderHistory = observer(() => { | |
| const { logEvent } = useAmplitudeAnalytics({ | |
| onLoadEvent: [EventName.LimitOrder.pageViewed], | |
| }); | |
| const { accountStore } = useStore(); | |
| const { t } = useTranslation(); | |
| const wallet = accountStore.getWallet(accountStore.osmosisChainId); | |
| const { | |
| orders, | |
| isLoading, | |
| fetchNextPage, | |
| isFetchingNextPage, | |
| hasNextPage, | |
| refetch, | |
| isRefetching, | |
| } = useOrderbookAllActiveOrders({ | |
| userAddress: wallet?.address ?? "", | |
| pageSize: 10, | |
| }); | |
| export const OrderHistory = observer(() => { | |
| const { logEvent } = useAmplitudeAnalytics({ | |
| onLoadEvent: [EventName.LimitOrder.pageViewed], | |
| }); | |
| const { accountStore } = useStore(); | |
| const { t } = useTranslation(); | |
| const wallet = accountStore.getWallet(accountStore.osmosisChainId); | |
| const { | |
| orders, | |
| isLoading, | |
| fetchNextPage, | |
| isFetchingNextPage, | |
| hasNextPage, | |
| refetch, | |
| isRefetching, | |
| error, | |
| } = useOrderbookAllActiveOrders({ | |
| userAddress: wallet?.address ?? "", | |
| pageSize: 10, | |
| }); | |
| if (isLoading) { | |
| return ( | |
| <div className="flex items-center justify-center py-10"> | |
| <Spinner className="!h-10 !w-10" /> | |
| </div> | |
| ); | |
| } | |
| if (error) { | |
| return <div>Error loading orders. Please try again later.</div>; | |
| } |
…ault [Limit Price - V3]: Price Input Default Values
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range, codebase verification and nitpick comments (2)
packages/web/components/place-limit-tool/limit-price-selector.tsx (2)
227-248: Ensure proper handling of input value changes.The
onChangehandlers forAutosizeInputshould properly handle edge cases and invalid inputs. Currently, they do not check if the input is a valid number after processing the input value.
- Location:
packages/web/components/place-limit-tool/limit-price-selector.tsx
- Line 227:
onChange={(e) => { const value = e.target.value.trim(); swapState.priceState.setPrice(value); }}- Line 248:
onChange={(e) => swapState.priceState.setPercentAdjusted(e.target.value.replace("%", "")) }Analysis chain
Ensure proper handling of input value changes.
The
onChangehandler forAutosizeInputshould properly handle edge cases and invalid inputs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper handling of input value changes. # Test: Search for the usage of `onChange` handler. Expect: Proper handling of edge cases and invalid inputs. rg --type tsx -A 5 $'onChange' packages/web/components/place-limit-tool/limit-price-selector.tsxLength of output: 123
Script:
#!/bin/bash # Description: Verify proper handling of input value changes. # Test: Search for the usage of `onChange` handler. Expect: Proper handling of edge cases and invalid inputs. rg -t js -A 5 $'onChange' packages/web/components/place-limit-tool/limit-price-selector.tsxLength of output: 504
1-21: Remove unused imports.The following imports are not used within the file and should be removed to avoid unnecessary dependencies:
@keplr-wallet/unitclassnamesnuqsreactreact-input-autosizereact-use~/components/assets~/components/loaders~/components/tooltip/generic-disclaimer~/hooks~/hooks/limit-orders~/utils/formatter~/utils/numberAnalysis chain
Remove unused imports if any.
Ensure that all imported modules are used within the file to avoid unnecessary dependencies.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unused imports in the file. # Test: Search for the usage of each imported module. Expect: All imported modules are used. rg --type tsx -A 5 $'import' packages/web/components/place-limit-tool/limit-price-selector.tsxLength of output: 121
Script:
#!/bin/bash # Description: Check for unused imports in the file. # Test: Search for the usage of each imported module. Expect: All imported modules are used. rg --type ts -A 5 $'import' packages/web/components/place-limit-tool/limit-price-selector.tsxLength of output: 999
Script:
#!/bin/bash # Description: Check for usage of each imported module in the file. file="packages/web/components/place-limit-tool/limit-price-selector.tsx" modules=( "@keplr-wallet/unit" "classnames" "nuqs" "react" "react-input-autosize" "react-use" "~/components/assets" "~/components/loaders" "~/components/tooltip/generic-disclaimer" "~/hooks" "~/hooks/limit-orders" "~/utils/formatter" "~/utils/number" ) for module in "${modules[@]}"; do echo "Searching for usage of module: $module" rg "$module" "$file" doneLength of output: 4194
|
|
||
| // Adjust order price as spot price changes until user inputs a price | ||
| useEffect(() => { | ||
| if (inputMode === InputMode.Price && !priceState.priceLocked) { | ||
| const formattedPrice = formatPretty( | ||
| priceState.spotPrice, | ||
| getPriceExtendedFormatOptions(priceState.spotPrice) | ||
| ).replace(/,/g, ""); | ||
| priceState._setPriceUnsafe(formattedPrice); | ||
| priceState._setPercentAdjustedUnsafe("0"); | ||
| } | ||
|
|
||
| if (inputMode === InputMode.Percentage) { | ||
| priceState.setPriceAsPercentageOfSpotPrice( | ||
| new Dec( | ||
| !!priceState.manualPercentAdjusted | ||
| ? priceState.manualPercentAdjusted.replace(/,/g, "") | ||
| : 0 | ||
| ).quo(new Dec(100)), | ||
| false | ||
| ); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [ | ||
| inputMode, | ||
| priceState.priceFiat, | ||
| priceState.priceLocked, | ||
| priceState.spotPrice, | ||
| ]); |
There was a problem hiding this comment.
Avoid using eslint-disable-next-line.
Avoid using eslint-disable-next-line as it can hide potential issues. Instead, refactor the code to comply with ESLint rules.
- // eslint-disable-next-line react-hooks/exhaustive-depsCommittable suggestion was skipped due to low confidence.
Tools
Biome
[error] 77-77: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
| const percentageSuffix = useMemo(() => { | ||
| return priceState.percentAdjusted.isZero() | ||
| ? `${ | ||
| orderDirection === "bid" | ||
| ? t("limitOrders.below") | ||
| : t("limitOrders.above") | ||
| } ${t("limitOrders.currentPrice")}` | ||
| : `${ | ||
| priceState.percentAdjusted.isNegative() | ||
| ? t("limitOrders.below") | ||
| : t("limitOrders.above") | ||
| } ${t("limitOrders.currentPrice")}`; | ||
| }, [t, priceState.percentAdjusted, orderDirection]); |
There was a problem hiding this comment.
Optimize percentageSuffix calculation.
The percentageSuffix calculation can be simplified for better readability.
- return priceState.percentAdjusted.isZero()
- ? `${
- orderDirection === "bid"
- ? t("limitOrders.below")
- : t("limitOrders.above")
- } ${t("limitOrders.currentPrice")}`
- : `${
- priceState.percentAdjusted.isNegative()
- ? t("limitOrders.below")
- : t("limitOrders.above")
- } ${t("limitOrders.currentPrice")}`;
+ if (priceState.percentAdjusted.isZero()) {
+ return `${
+ orderDirection === "bid"
+ ? t("limitOrders.below")
+ : t("limitOrders.above")
+ } ${t("limitOrders.currentPrice")}`;
+ }
+ return `${
+ priceState.percentAdjusted.isNegative()
+ ? t("limitOrders.below")
+ : t("limitOrders.above")
+ } ${t("limitOrders.currentPrice")}`;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 percentageSuffix = useMemo(() => { | |
| return priceState.percentAdjusted.isZero() | |
| ? `${ | |
| orderDirection === "bid" | |
| ? t("limitOrders.below") | |
| : t("limitOrders.above") | |
| } ${t("limitOrders.currentPrice")}` | |
| : `${ | |
| priceState.percentAdjusted.isNegative() | |
| ? t("limitOrders.below") | |
| : t("limitOrders.above") | |
| } ${t("limitOrders.currentPrice")}`; | |
| }, [t, priceState.percentAdjusted, orderDirection]); | |
| const percentageSuffix = useMemo(() => { | |
| if (priceState.percentAdjusted.isZero()) { | |
| return `${ | |
| orderDirection === "bid" | |
| ? t("limitOrders.below") | |
| : t("limitOrders.above") | |
| } ${t("limitOrders.currentPrice")}`; | |
| } | |
| return `${ | |
| priceState.percentAdjusted.isNegative() | |
| ? t("limitOrders.below") | |
| : t("limitOrders.above") | |
| } ${t("limitOrders.currentPrice")}`; | |
| }, [t, priceState.percentAdjusted, orderDirection]); |
| const priceLabel = useMemo(() => { | ||
| if (inputMode === InputMode.Percentage) { | ||
| return formatPretty( | ||
| priceState.priceFiat, | ||
| getPriceExtendedFormatOptions(priceState.priceFiat.toDec()) | ||
| ); | ||
| } | ||
|
|
||
| return priceState.percentAdjusted.isZero() | ||
| ? t("limitOrders.marketPrice") | ||
| : `${formatPretty(priceState.percentAdjusted.mul(new Dec(100)).abs(), { | ||
| maxDecimals: 3, | ||
| maximumSignificantDigits: 5, | ||
| })}%`; | ||
| }, [inputMode, priceState.percentAdjusted, priceState.priceFiat, t]); |
There was a problem hiding this comment.
Optimize priceLabel calculation.
The priceLabel calculation can be simplified for better readability.
- return priceState.percentAdjusted.isZero()
- ? t("limitOrders.marketPrice")
- : `${formatPretty(priceState.percentAdjusted.mul(new Dec(100)).abs(), {
- maxDecimals: 3,
- maximumSignificantDigits: 5,
- })}%`;
+ if (priceState.percentAdjusted.isZero()) {
+ return t("limitOrders.marketPrice");
+ }
+ return `${formatPretty(priceState.percentAdjusted.mul(new Dec(100)).abs(), {
+ maxDecimals: 3,
+ maximumSignificantDigits: 5,
+ })}%`;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 priceLabel = useMemo(() => { | |
| if (inputMode === InputMode.Percentage) { | |
| return formatPretty( | |
| priceState.priceFiat, | |
| getPriceExtendedFormatOptions(priceState.priceFiat.toDec()) | |
| ); | |
| } | |
| return priceState.percentAdjusted.isZero() | |
| ? t("limitOrders.marketPrice") | |
| : `${formatPretty(priceState.percentAdjusted.mul(new Dec(100)).abs(), { | |
| maxDecimals: 3, | |
| maximumSignificantDigits: 5, | |
| })}%`; | |
| }, [inputMode, priceState.percentAdjusted, priceState.priceFiat, t]); | |
| const priceLabel = useMemo(() => { | |
| if (inputMode === InputMode.Percentage) { | |
| return formatPretty( | |
| priceState.priceFiat, | |
| getPriceExtendedFormatOptions(priceState.priceFiat.toDec()) | |
| ); | |
| } | |
| if (priceState.percentAdjusted.isZero()) { | |
| return t("limitOrders.marketPrice"); | |
| } | |
| return `${formatPretty(priceState.percentAdjusted.mul(new Dec(100)).abs(), { | |
| maxDecimals: 3, | |
| maximumSignificantDigits: 5, | |
| })}%`; | |
| }, [inputMode, priceState.percentAdjusted, priceState.priceFiat, t]); |
| export const LimitPriceSelector: FC<LimitPriceSelectorProps> = ({ | ||
| swapState, | ||
| orderDirection, | ||
| }) => { | ||
| const [tab] = useQueryState("tab", parseAsString.withDefault("swap")); | ||
| const [input, setInput] = useState<HTMLInputElement | null>(null); | ||
| const { t } = useTranslation(); | ||
| const { priceState } = swapState; | ||
| const [inputMode, setInputMode] = useState(InputMode.Price); | ||
|
|
||
| const swapInputMode = useCallback(() => { | ||
| setInputMode( | ||
| inputMode === InputMode.Percentage | ||
| ? InputMode.Price | ||
| : InputMode.Percentage | ||
| ); | ||
|
|
||
| if (inputMode === InputMode.Price) { | ||
| priceState.setPriceLock(false); | ||
| } else { | ||
| priceState.setPriceLock(true); | ||
| } | ||
|
|
||
| if (priceState.isBeyondOppositePrice || !priceState.manualPercentAdjusted) { | ||
| priceState.setPercentAdjusted("0"); | ||
| } | ||
|
|
||
| if (input) input.focus(); | ||
| }, [inputMode, input, priceState]); |
There was a problem hiding this comment.
Ensure proper cleanup of side effects.
The useCallback hook for swapInputMode does not clean up side effects. Ensure proper cleanup to avoid memory leaks.
- const swapInputMode = useCallback(() => {
+ const swapInputMode = useCallback(() => {
...
- }, [inputMode, input, priceState]);
+ }, [inputMode, input, priceState]);
+ useEffect(() => {
+ return () => {
+ // Cleanup code here
+ };
+ }, [swapInputMode]);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.
| export const LimitPriceSelector: FC<LimitPriceSelectorProps> = ({ | |
| swapState, | |
| orderDirection, | |
| }) => { | |
| const [tab] = useQueryState("tab", parseAsString.withDefault("swap")); | |
| const [input, setInput] = useState<HTMLInputElement | null>(null); | |
| const { t } = useTranslation(); | |
| const { priceState } = swapState; | |
| const [inputMode, setInputMode] = useState(InputMode.Price); | |
| const swapInputMode = useCallback(() => { | |
| setInputMode( | |
| inputMode === InputMode.Percentage | |
| ? InputMode.Price | |
| : InputMode.Percentage | |
| ); | |
| if (inputMode === InputMode.Price) { | |
| priceState.setPriceLock(false); | |
| } else { | |
| priceState.setPriceLock(true); | |
| } | |
| if (priceState.isBeyondOppositePrice || !priceState.manualPercentAdjusted) { | |
| priceState.setPercentAdjusted("0"); | |
| } | |
| if (input) input.focus(); | |
| }, [inputMode, input, priceState]); | |
| export const LimitPriceSelector: FC<LimitPriceSelectorProps> = ({ | |
| swapState, | |
| orderDirection, | |
| }) => { | |
| const [tab] = useQueryState("tab", parseAsString.withDefault("swap")); | |
| const [input, setInput] = useState<HTMLInputElement | null>(null); | |
| const { t } = useTranslation(); | |
| const { priceState } = swapState; | |
| const [inputMode, setInputMode] = useState(InputMode.Price); | |
| const swapInputMode = useCallback(() => { | |
| setInputMode( | |
| inputMode === InputMode.Percentage | |
| ? InputMode.Price | |
| : InputMode.Percentage | |
| ); | |
| if (inputMode === InputMode.Price) { | |
| priceState.setPriceLock(false); | |
| } else { | |
| priceState.setPriceLock(true); | |
| } | |
| if (priceState.isBeyondOppositePrice || !priceState.manualPercentAdjusted) { | |
| priceState.setPercentAdjusted("0"); | |
| } | |
| if (input) input.focus(); | |
| }, [inputMode, input, priceState]); | |
| useEffect(() => { | |
| return () => { | |
| // Cleanup code here | |
| }; | |
| }, [swapInputMode]); |
| export interface UsePlaceLimitParams { | ||
| osmosisChainId: string; | ||
| orderDirection: OrderDirection; | ||
| useQueryParams?: boolean; | ||
| useOtherCurrencies?: boolean; | ||
| baseDenom: string; | ||
| quoteDenom: string; | ||
| type: "limit" | "market"; | ||
| page: EventPage; | ||
| maxSlippage?: Dec; | ||
| } |
There was a problem hiding this comment.
Ensure consistent prop types and default values.
Ensure that the prop types and default values are consistent and clearly defined. Consider using TypeScript interfaces for better type safety.
export interface UsePlaceLimitParams {
osmosisChainId: string;
orderDirection: OrderDirection;
useQueryParams?: boolean;
useOtherCurrencies?: boolean;
baseDenom: string;
quoteDenom: string;
type: "limit" | "market";
page: EventPage;
maxSlippage?: Dec;
}| // TODO: adjust as necessary | ||
| const CLAIM_BOUNTY = "0.0001"; |
There was a problem hiding this comment.
Reminder: Address TODO comments.
The TODO comment indicates potential improvements.
Do you want me to address the TODO comments or open a GitHub issue to track this task?
| const useLimitPrice = ({ | ||
| orderDirection, | ||
| baseDenom, | ||
| }: { | ||
| orderDirection: OrderDirection; | ||
| baseDenom?: string; | ||
| }) => { |
There was a problem hiding this comment.
Ensure consistent prop types and default values.
Ensure that the prop types and default values are consistent and clearly defined. Consider using TypeScript interfaces for better type safety.
interface UseLimitPriceParams {
orderbookContractAddress: string;
orderDirection: OrderDirection;
baseDenom?: string;
}| // The raw percentage adjusted based on the current order price state | ||
| const percentAdjusted = useMemo( | ||
| () => | ||
| !!manualPercentAdjusted |
There was a problem hiding this comment.
Remove redundant double-negation.
The double-negation is unnecessary and can be removed.
- !!manualPercentAdjusted
+ manualPercentAdjustedCommittable 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.
| !!manualPercentAdjusted | |
| manualPercentAdjusted |
Tools
Biome
[error] 795-795: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
| // Multiply by 10^n where n is the amount of decimals for the quote asset | ||
| const quoteTokenAmount = outgoingFiatValue! | ||
| .quo(quoteAssetPrice.toDec() ?? new Dec(1)) | ||
| .mul(new Dec(Math.pow(10, quoteAsset!.coinDecimals))); |
There was a problem hiding this comment.
Use exponentiation operator for clarity and modern JavaScript practices.
Replace Math.pow with the exponentiation operator ** for better readability and to adhere to modern JavaScript standards.
- .mul(new Dec(Math.pow(10, quoteAsset!.coinDecimals)));
+ .mul(new Dec(10 ** quoteAsset!.coinDecimals));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.
| .mul(new Dec(Math.pow(10, quoteAsset!.coinDecimals))); | |
| .mul(new Dec(10 ** quoteAsset!.coinDecimals)); |
| (percentAdjusted: string) => { | ||
| if (percentAdjusted.startsWith(".")) { | ||
| percentAdjusted = "0" + percentAdjusted; | ||
| } | ||
|
|
||
| if ( | ||
| percentAdjusted.length > 0 && | ||
| !isValidNumericalRawInput(percentAdjusted) | ||
| ) | ||
| return; | ||
|
|
||
| if (countDecimals(percentAdjusted) > 3) { | ||
| // percentAdjusted = parseFloat(percentAdjusted).toFixed(10).toString(); | ||
| return; | ||
| } | ||
|
|
||
| const split = percentAdjusted.split("."); | ||
| if (split[0].length > 9) { | ||
| return; | ||
| } | ||
|
|
||
| // Do not allow the user to input 100% below current price | ||
| if ( | ||
| orderDirection === "bid" && | ||
| percentAdjusted.length > 0 && | ||
| new Dec(percentAdjusted).gte(new Dec(100)) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| setManualPercentAdjusted(percentAdjusted); | ||
|
|
||
| if (!percentAdjusted) { | ||
| setPriceAsPercentageOfSpotPrice(new Dec(0), false); | ||
| return; | ||
| } | ||
|
|
||
| setPriceAsPercentageOfSpotPrice( | ||
| new Dec(percentAdjusted).quo(new Dec(100)), | ||
| false | ||
| ); |
There was a problem hiding this comment.
Optimize nested if conditions for better readability.
The nested if conditions can be optimized for better readability.
- if (percentAdjusted.startsWith(".")) {
- percentAdjusted = "0" + percentAdjusted;
- }
- if (percentAdjusted.length > 0 && !isValidNumericalRawInput(percentAdjusted)) {
- return;
- }
- if (countDecimals(percentAdjusted) > 3) {
- return;
- }
- const split = percentAdjusted.split(".");
- if (split[0].length > 9) {
- return;
- }
- if (orderDirection === "bid" && percentAdjusted.length > 0 && new Dec(percentAdjusted).gte(new Dec(100))) {
- return;
- }
+ if (percentAdjusted.startsWith(".")) {
+ percentAdjusted = "0" + percentAdjusted;
+ } else if (percentAdjusted.length > 0 && !isValidNumericalRawInput(percentAdjusted)) {
+ return;
+ } else if (countDecimals(percentAdjusted) > 3) {
+ return;
+ } else if (percentAdjusted.split(".")[0].length > 9) {
+ return;
+ } else if (orderDirection === "bid" && percentAdjusted.length > 0 && new Dec(percentAdjusted).gte(new Dec(100))) {
+ return;
+ }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.
| (percentAdjusted: string) => { | |
| if (percentAdjusted.startsWith(".")) { | |
| percentAdjusted = "0" + percentAdjusted; | |
| } | |
| if ( | |
| percentAdjusted.length > 0 && | |
| !isValidNumericalRawInput(percentAdjusted) | |
| ) | |
| return; | |
| if (countDecimals(percentAdjusted) > 3) { | |
| // percentAdjusted = parseFloat(percentAdjusted).toFixed(10).toString(); | |
| return; | |
| } | |
| const split = percentAdjusted.split("."); | |
| if (split[0].length > 9) { | |
| return; | |
| } | |
| // Do not allow the user to input 100% below current price | |
| if ( | |
| orderDirection === "bid" && | |
| percentAdjusted.length > 0 && | |
| new Dec(percentAdjusted).gte(new Dec(100)) | |
| ) { | |
| return; | |
| } | |
| setManualPercentAdjusted(percentAdjusted); | |
| if (!percentAdjusted) { | |
| setPriceAsPercentageOfSpotPrice(new Dec(0), false); | |
| return; | |
| } | |
| setPriceAsPercentageOfSpotPrice( | |
| new Dec(percentAdjusted).quo(new Dec(100)), | |
| false | |
| ); | |
| (percentAdjusted: string) => { | |
| if (percentAdjusted.startsWith(".")) { | |
| percentAdjusted = "0" + percentAdjusted; | |
| } else if (percentAdjusted.length > 0 && !isValidNumericalRawInput(percentAdjusted)) { | |
| return; | |
| } else if (countDecimals(percentAdjusted) > 3) { | |
| return; | |
| } else if (percentAdjusted.split(".")[0].length > 9) { | |
| return; | |
| } else if (orderDirection === "bid" && percentAdjusted.length > 0 && new Dec(percentAdjusted).gte(new Dec(100))) { | |
| return; | |
| } | |
| setManualPercentAdjusted(percentAdjusted); | |
| if (!percentAdjusted) { | |
| setPriceAsPercentageOfSpotPrice(new Dec(0), false); | |
| return; | |
| } | |
| setPriceAsPercentageOfSpotPrice( | |
| new Dec(percentAdjusted).quo(new Dec(100)), | |
| false | |
| ); |
| function getNormalizationFactor( | ||
| baseAssetDecimals: number, | ||
| quoteAssetDecimals: number | ||
| ) { | ||
| return new Dec(10).pow(new Int(quoteAssetDecimals - baseAssetDecimals)); | ||
| } |
There was a problem hiding this comment.
Use exponentiation operator for clarity and modern JavaScript practices.
Replace Math.pow with the exponentiation operator ** for better readability and to adhere to modern JavaScript standards.
- return new Dec(10).pow(new Int(quoteAssetDecimals - baseAssetDecimals));
+ return new Dec(10 ** (quoteAssetDecimals - baseAssetDecimals));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.
| function getNormalizationFactor( | |
| baseAssetDecimals: number, | |
| quoteAssetDecimals: number | |
| ) { | |
| return new Dec(10).pow(new Int(quoteAssetDecimals - baseAssetDecimals)); | |
| } | |
| function getNormalizationFactor( | |
| baseAssetDecimals: number, | |
| quoteAssetDecimals: number | |
| ) { | |
| return new Dec(10 ** (quoteAssetDecimals - baseAssetDecimals)); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (5)
packages/web/components/trade-tool/index.tsx (2)
21-24: Consider replacing the interface with a type alias.The interface
TradeToolPropsis simple and could be replaced with a type alias for better clarity.- export interface TradeToolProps { - swapToolProps?: SwapToolProps; - page: EventPage; - } + export type TradeToolProps = { + swapToolProps?: SwapToolProps; + page: EventPage; + };
46-57: Avoid usingeslint-disable-next-line.Avoid using
eslint-disable-next-lineas it can hide potential issues. Instead, refactor the code to comply with ESLint rules.- // eslint-disable-next-line react-hooks/exhaustive-depspackages/trpc/src/orderbook-router.ts (2)
34-47: Refactor for better readability.The
defaultSortOrdersfunction can be simplified using a ternary operator.function defaultSortOrders( orderA: MappedLimitOrder, orderB: MappedLimitOrder ): number { if (orderA.status === orderB.status) { return orderB.placed_at - orderA.placed_at; } return orderStatusOrder[orderA.status] < orderStatusOrder[orderB.status] ? -1 : 1; }
128-160: Ensure proper error handling in asynchronous calls.The
getClaimableOrders,getOrderbookDenoms, andgetOrderbookActiveOrdersfunctions do not include any error handling mechanisms. It is crucial to add try-catch blocks or other error handling logic to manage potential errors and provide meaningful error messages.
- File:
packages/trpc/src/orderbook-router.ts- Lines: 128-160
Analysis chain
Verify error handling.
Ensure that all asynchronous calls within the
getClaimableOrdersfunction handle errors properly and return meaningful error messages if any call fails.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in asynchronous calls within `getClaimableOrders` function. # Test: Search for error handling in asynchronous calls. Expect: Proper error handling is implemented. rg --type js 'getOrderbookDenoms|getOrderbookActiveOrders' -A 10Length of output: 66
Script:
#!/bin/bash # Description: Verify error handling in `getClaimableOrders` function and its dependent functions. # Search for the `getClaimableOrders` function implementation. ast-grep --lang typescript --pattern $'const getClaimableOrders: $_ = $_.query(async ({ input, ctx }) => { $$$ })' # Search for the `getOrderbookDenoms` function implementation. ast-grep --lang typescript --pattern $'async function getOrderbookDenoms($_) { $$$ }' # Search for the `getOrderbookActiveOrders` function implementation. ast-grep --lang typescript --pattern $'async function getOrderbookActiveOrders($_) { $$$ }'Length of output: 295
Script:
#!/bin/bash # Description: Verify error handling in `getClaimableOrders` function and its dependent functions. # Search for the `getClaimableOrders` function by its name. rg 'getClaimableOrders' -A 20 # Search for the `getOrderbookDenoms` function by its name. rg 'getOrderbookDenoms' -A 20 # Search for the `getOrderbookActiveOrders` function by its name. rg 'getOrderbookActiveOrders' -A 20Length of output: 20364
Script:
#!/bin/bash # Description: Retrieve full implementations to verify error handling in `getClaimableOrders` function and its dependent functions. # Retrieve the full implementation of the `getClaimableOrders` function. rg 'getClaimableOrders' -A 50 # Retrieve the full implementation of the `getOrderbookDenoms` function. rg 'getOrderbookDenoms' -A 50 # Retrieve the full implementation of the `getOrderbookActiveOrders` function. rg 'getOrderbookActiveOrders' -A 50Length of output: 44060
Tools
Biome
[error] 158-158: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
packages/web/components/place-limit-tool/limit-price-selector.tsx (1)
33-36: Ensure proper cleanup of side effects.The
useCallbackhook forswapInputModedoes not clean up side effects. Ensure proper cleanup to avoid memory leaks.- const swapInputMode = useCallback(() => { + const swapInputMode = useCallback(() => { ... - }, [inputMode, input, priceState]); + }, [inputMode, </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| }} | ||
| /> | ||
| ); | ||
| case SwapToolTab.SWAP: |
There was a problem hiding this comment.
Remove the redundant case clause in the switch statement.
The case SwapToolTab.SWAP is redundant as it falls through to the default case. Removing it simplifies the code and reduces unnecessary complexity.
- case SwapToolTab.SWAP: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.
| case SwapToolTab.SWAP: |
Tools
Biome
[error] 109-109: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
| {useMemo(() => { | ||
| switch (tab) { | ||
| case SwapToolTab.BUY: | ||
| return ( | ||
| <PlaceLimitTool | ||
| key="tool-buy" | ||
| page={page} | ||
| refetchOrders={async () => { | ||
| refetch({ stale: false }); | ||
| }} | ||
| /> | ||
| ); | ||
| case SwapToolTab.SELL: | ||
| return ( | ||
| <PlaceLimitTool | ||
| key="tool-sell" | ||
| page={page} | ||
| refetchOrders={async () => { | ||
| refetch({ stale: false }); | ||
| }} | ||
| /> | ||
| ); | ||
| case SwapToolTab.SWAP: | ||
| default: | ||
| return ( | ||
| <AltSwapTool | ||
| useOtherCurrencies | ||
| useQueryParams | ||
| page={page} | ||
| {...swapToolProps} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Optimize memoization in the return statement.
The memoization could be optimized by extracting the memoized component into a separate function.
- {useMemo(() => {
- switch (tab) {
- case SwapToolTab.BUY:
- return <PlaceLimitTool key="tool-buy" page={page} refetchOrders={async () => { refetch({ stale: false }); }} />;
- case SwapToolTab.SELL:
- return <PlaceLimitTool key="tool-sell" page={page} refetchOrders={async () => { refetch({ stale: false }); }} />;
- case SwapToolTab.SWAP:
- default:
- return <AltSwapTool useOtherCurrencies useQueryParams page={page} {...swapToolProps} />;
- }
- }, [page, swapToolProps, tab, refetch])}
+ const renderTool = useMemo(() => {
+ switch (tab) {
+ case SwapToolTab.BUY:
+ return <PlaceLimitTool key="tool-buy" page={page} refetchOrders={async () => { refetch({ stale: false }); }} />;
+ case SwapToolTab.SELL:
+ return <PlaceLimitTool key="tool-sell" page={page} refetchOrders={async () => { refetch({ stale: false }); }} />;
+ case SwapToolTab.SWAP:
+ default:
+ return <AltSwapTool useOtherCurrencies useQueryParams page={page} {...swapToolProps} />;
+ }
+ }, [page, swapToolProps, tab, refetch]);
+ return (
+ <ClientOnly>
+ <div className="relative flex flex-col gap-3 rounded-3xl bg-osmoverse-900 px-5 pt-5 pb-3">
+ <div className="flex w-full items-center justify-between md:flex-wrap md:gap-4">
+ <SwapToolTabs activeTab={tab} setTab={setTab} />
+ <div className="flex items-center gap-2">
+ {tab !== SwapToolTab.SWAP && <OrderTypeSelector />}
+ {/* {wallet?.isWalletConnected && (
+ <Link
+ href={"/transactions?tab=orders&fromPage=swap"}
+ className="relative flex h-12 w-12 items-center justify-center overflow-visible rounded-full bg-osmoverse-825 transition-colors hover:bg-osmoverse-700"
+ >
+ <Icon
+ id="history-uncolored"
+ width={24}
+ height={24}
+ className="h-6 w-6 text-wosmongton-200"
+ />
+ {count > 0 && (
+ <div className="absolute -top-1 -right-1 flex h-6 w-6 items-center justify-center rounded-full bg-[#A51399]">
+ <span className="text-xs leading-[14px]">{count}</span>
+ </div>
+ )}
+ </Link>
+ )} */}
+ </div>
+ </div>
+ {renderTool}
+ </div>
+ {wallet?.isWalletConnected && orders.length > 0 && (
+ <Link
+ href="/transactions?tab=orders&fromPage=swap"
+ className="my-3 flex items-center justify-between rounded-2xl border border-solid border-osmoverse-800/50 bg-osmoverse-1000 py-2 px-4 hover:bg-osmoverse-850"
+ >
+ <div className="flex items-center gap-3">
+ <div className="flex h-10 w-10 items-center justify-center">
+ <Icon
+ id="history-uncolored"
+ width={24}
+ height={24}
+ className="text-osmoverse-400"
+ />
+ </div>
+ <span className="subtitle1 text-osmoverse-300">
+ {t("limitOrders.orderHistory")}
+ </span>
+ </div>
+ <div className="flex items-center gap-2">
+ {/* <span className="text-wosmongton-200">{openOrders.length}</span> */}
+ <div className="flex h-6 w-6 items-center justify-center">
+ <Icon
+ id="chevron-right"
+ width={7}
+ height={12}
+ className="text-osmoverse-400"
+ />
+ </div>
+ </div>
+ </Link>
+ )}
+ </ClientOnly>
+ );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.
| {useMemo(() => { | |
| switch (tab) { | |
| case SwapToolTab.BUY: | |
| return ( | |
| <PlaceLimitTool | |
| key="tool-buy" | |
| page={page} | |
| refetchOrders={async () => { | |
| refetch({ stale: false }); | |
| }} | |
| /> | |
| ); | |
| case SwapToolTab.SELL: | |
| return ( | |
| <PlaceLimitTool | |
| key="tool-sell" | |
| page={page} | |
| refetchOrders={async () => { | |
| refetch({ stale: false }); | |
| }} | |
| /> | |
| ); | |
| case SwapToolTab.SWAP: | |
| default: | |
| return ( | |
| <AltSwapTool | |
| useOtherCurrencies | |
| useQueryParams | |
| page={page} | |
| {...swapToolProps} | |
| /> | |
| ); | |
| } | |
| const renderTool = useMemo(() => { | |
| switch (tab) { | |
| case SwapToolTab.BUY: | |
| return <PlaceLimitTool key="tool-buy" page={page} refetchOrders={async () => { refetch({ stale: false }); }} />; | |
| case SwapToolTab.SELL: | |
| return <PlaceLimitTool key="tool-sell" page={page} refetchOrders={async () => { refetch({ stale: false }); }} />; | |
| case SwapToolTab.SWAP: | |
| default: | |
| return <AltSwapTool useOtherCurrencies useQueryParams page={page} {...swapToolProps} />; | |
| } | |
| }, [page, swapToolProps, tab, refetch]); | |
| return ( | |
| <ClientOnly> | |
| <div className="relative flex flex-col gap-3 rounded-3xl bg-osmoverse-900 px-5 pt-5 pb-3"> | |
| <div className="flex w-full items-center justify-between md:flex-wrap md:gap-4"> | |
| <SwapToolTabs activeTab={tab} setTab={setTab} /> | |
| <div className="flex items-center gap-2"> | |
| {tab !== SwapToolTab.SWAP && <OrderTypeSelector />} | |
| {/* {wallet?.isWalletConnected && ( | |
| <Link | |
| href={"/transactions?tab=orders&fromPage=swap"} | |
| className="relative flex h-12 w-12 items-center justify-center overflow-visible rounded-full bg-osmoverse-825 transition-colors hover:bg-osmoverse-700" | |
| > | |
| <Icon | |
| id="history-uncolored" | |
| width={24} | |
| height={24} | |
| className="h-6 w-6 text-wosmongton-200" | |
| /> | |
| {count > 0 && ( | |
| <div className="absolute -top-1 -right-1 flex h-6 w-6 items-center justify-center rounded-full bg-[#A51399]"> | |
| <span className="text-xs leading-[14px]">{count}</span> | |
| </div> | |
| )} | |
| </Link> | |
| )} */} | |
| </div> | |
| </div> | |
| {renderTool} | |
| </div> | |
| {wallet?.isWalletConnected && orders.length > 0 && ( | |
| <Link | |
| href="/transactions?tab=orders&fromPage=swap" | |
| className="my-3 flex items-center justify-between rounded-2xl border border-solid border-osmoverse-800/50 bg-osmoverse-1000 py-2 px-4 hover:bg-osmoverse-850" | |
| > | |
| <div className="flex items-center gap-3"> | |
| <div className="flex h-10 w-10 items-center justify-center"> | |
| <Icon | |
| id="history-uncolored" | |
| width={24} | |
| height={24} | |
| className="text-osmoverse-400" | |
| /> | |
| </div> | |
| <span className="subtitle1 text-osmoverse-300"> | |
| {t("limitOrders.orderHistory")} | |
| </span> | |
| </div> | |
| <div className="flex items-center gap-2"> | |
| {/* <span className="text-wosmongton-200">{openOrders.length}</span> */} | |
| <div className="flex h-6 w-6 items-center justify-center"> | |
| <Icon | |
| id="chevron-right" | |
| width={7} | |
| height={12} | |
| className="text-osmoverse-400" | |
| /> | |
| </div> | |
| </div> | |
| </Link> | |
| )} | |
| </ClientOnly> | |
| ); |
Tools
Biome
[error] 109-109: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
| getClaimableOrders: publicProcedure | ||
| .input( | ||
| z | ||
| .object({ contractAddresses: z.array(z.string().startsWith("osmo")) }) | ||
| .required() | ||
| .and(UserOsmoAddressSchema.required()) | ||
| ) | ||
| .query(async ({ input, ctx }) => { | ||
| const { contractAddresses, userOsmoAddress } = input; | ||
| const promises = contractAddresses.map( | ||
| async (contractOsmoAddress: string) => { | ||
| const { quoteAsset, baseAsset } = await getOrderbookDenoms({ | ||
| orderbookAddress: contractOsmoAddress, | ||
| chainList: ctx.chainList, | ||
| assetLists: ctx.assetLists, | ||
| }); | ||
| const orders = await getOrderbookActiveOrders({ | ||
| orderbookAddress: contractOsmoAddress, | ||
| userOsmoAddress: userOsmoAddress, | ||
| chainList: ctx.chainList, | ||
| baseAsset, | ||
| quoteAsset, | ||
| }); | ||
|
|
||
| if (orders.length === 0) return []; | ||
|
|
||
| return orders.filter((o) => o.percentFilled.gte(new Dec(1))); | ||
| } | ||
| ); | ||
| const ordersByContracts = await Promise.all(promises); | ||
| const allOrders = ordersByContracts.flatMap((p) => p); | ||
| return allOrders; | ||
| }), |
There was a problem hiding this comment.
Optimize array flattening with flat.
The use of flatMap is unnecessary for simple array flattening. Replace it with flat for better performance and clarity.
- const allOrders = ordersByContracts.flatMap((p) => p);
+ const allOrders = ordersByContracts.flat();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.
| getClaimableOrders: publicProcedure | |
| .input( | |
| z | |
| .object({ contractAddresses: z.array(z.string().startsWith("osmo")) }) | |
| .required() | |
| .and(UserOsmoAddressSchema.required()) | |
| ) | |
| .query(async ({ input, ctx }) => { | |
| const { contractAddresses, userOsmoAddress } = input; | |
| const promises = contractAddresses.map( | |
| async (contractOsmoAddress: string) => { | |
| const { quoteAsset, baseAsset } = await getOrderbookDenoms({ | |
| orderbookAddress: contractOsmoAddress, | |
| chainList: ctx.chainList, | |
| assetLists: ctx.assetLists, | |
| }); | |
| const orders = await getOrderbookActiveOrders({ | |
| orderbookAddress: contractOsmoAddress, | |
| userOsmoAddress: userOsmoAddress, | |
| chainList: ctx.chainList, | |
| baseAsset, | |
| quoteAsset, | |
| }); | |
| if (orders.length === 0) return []; | |
| return orders.filter((o) => o.percentFilled.gte(new Dec(1))); | |
| } | |
| ); | |
| const ordersByContracts = await Promise.all(promises); | |
| const allOrders = ordersByContracts.flatMap((p) => p); | |
| return allOrders; | |
| }), | |
| const allOrders = ordersByContracts.flat(); |
Tools
Biome
[error] 158-158: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
[Limit Orders - V3]: Font Scaling
What is the purpose of the change:
These changes implement limit orders for the swap page. Limit orders are an alternative method for a user to buy/sell their assets, as such the swap widget has been replaced with one that allows the user to swap between limit, market and swap orders as an extension of the previous implementation.
For limit and market orders an orderbook smart contract is used. This is recognised as a standard pool and as such a market order can be run as a swap on the desired orderbook. For a limit order the inputs are more complex, the user should be able to select their buy/sell denoms as usual but also be able to select a price, both in raw value and percentage based on the current market price.
The input field has also been updated for limit orders, the user can enter both a desired fiat amount (in USD) and a token amount while being able to hotswap between the two. The price management is handled in
useLimitPricewhile the entire limit placing state is handled inusePlaceLimit. As the available denominations for limit and market orders is more restricted and the components have a much different design they have been separated in to new components.To aid in maintaining correct state a new edge router was added (the
orderbookRouter), this contains queries for the current state of the orderbook and any active orders the user may have. Orders may be active on several different orderbooks, these are queried individually and amalgamated in a single paginated response.Linear Task
Implement Limit Orders
Brief Changelog
TRPC
tickToPricecalculationorderbookRouterwith following queries (all are cached):getMakerFee(May be replaced by SQS eventually)getActiveOrders- the user's active orders for a specific orderbookgetAllActiveOrders- the user's active orders for a set of orderbooksgetSpotPrice(May be replaced by SQS eventually)Components
orders-historypage (A separate PR will be made for this)TokenSelectLimitcomponent - alternate token selection component for limit ordersLimitInputcomponent - alternate input element for limit/market ordersPlaceLimitToolcomponent - alternate to swap tool for placing limit/market ordersLimitPriceSelectorcomponent - allows the user to select a price for their limit orderLimitTradeDetailscomponent - brief display of fees and output for a limit orderSwapToolcomponent - only a UI change, no functionality differencesOrderTypeSelectorcomponent - swaps between limit and market ordersPriceSelectorcomponent - allows the user to select a quote denom for their limit/market orderSwapToolTabscomponent - allows selection of buy/sell/swap ordersTradeToolcomponent - replaces the currentSwapToolon the swap pageHooks
use-orderbookhooks which return data related to a selected orderbookuse-place-limithooks, this is similar touseSwapbut for limit ordersOther
use-swaphooks for use inuse-place-limit