feat(spaceward): fix sending cosmos not using the same rpc as in assets fetch data; fix handling collapsed transactions #892
Conversation
|
Hey @alex-nax and thank you for opening this pull request! 👋🏼 It looks like you forgot to add a changelog entry for your changes. Make sure to add a changelog entry in the 'CHANGELOG.md' file. |
|
Caution Review failedThe pull request is closed. WalkthroughWalkthroughThe changes in this pull request involve updates to several components and utility functions within the codebase. Key modifications include enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Outside diff range and nitpick comments (9)
spaceward/src/lib/eth/constants.ts (2)
4-4: LGTM! Consider using a type alias for better maintainability.The updated type definition for
ETH_CHAIN_CONFIGimproves the structure by providing more detailed information about tokens and allowing for a more descriptive representation of chains. This change is consistent with the modifications in the object literal below.Consider creating a type alias for the chain configuration to improve maintainability:
type ChainConfig = { rpc: string[]; token?: { symbol: string; name: string }; title?: string; }; export const ETH_CHAIN_CONFIG: Record<string, ChainConfig> = { // ... (rest of the code remains the same) };
36-38: LGTM! Consider adding atitleproperty for consistency.The addition of the Base chain configuration is a valuable update. The provided RPC endpoints and token information appear to be accurate and consistent with the new type definition.
For consistency with other chain configurations, consider adding a
titleproperty:"8453": { rpc: ["https://mainnet.base.org/", "https://base.llamarpc.com"], token: { symbol: "BASE", name: "Base" }, title: "Base", },spaceward/src/lib/eth/util.ts (2)
20-22: LGTM: New variables added with appropriate fallback values.The addition of
tokenSymbol,tokenName, andtitlevariables enhances the information provided by thegetProviderfunction. The use of nullish coalescing for fallback values is appropriate.Consider using object destructuring for cleaner code:
-const tokenSymbol = config?.token?.symbol ?? "ETH"; -const tokenName = config?.token?.name ?? "Ethereum"; +const { symbol: tokenSymbol = "ETH", name: tokenName = "Ethereum" } = config?.token ?? {};
Line range hint
18-59: Consider adding a type definition for the getProvider return value.The
getProviderfunction has been updated to return an object with additional properties. To improve type safety and documentation, consider adding a type definition for this return value.Add the following type definition before the
getProviderfunction:type ProviderInfo = { provider: ethers.JsonRpcProvider; tokenSymbol: string; tokenName: string; title: string; }; export const getProvider = (type: SupportedNetwork): ProviderInfo => { // ... existing function body ... };This will provide better type checking and autocompletion for consumers of the
getProviderfunction.spaceward/src/features/actions/util.ts (1)
387-388: Refine the error message for missingrpcEndpointConsider providing more context in the error message to aid in debugging. For example, specify which function or action requires the
rpcEndpoint.Apply this diff to enhance the error message:
if (!rpcEndpoint) { - throw new Error("missing rpcEndpoint"); + throw new Error("Missing rpcEndpoint: 'handleCosmos' requires a valid rpcEndpoint parameter."); }spaceward/src/features/actions/StatusSidebar.tsx (1)
447-447: Review initial state ofhideinStatusSidebarThe
hidestate is initialized totrue, which means thePopoverContentwill be hidden initially. If the intended default behavior is to show the popover when there are multiple transactions, this may not align with your goal.Consider initializing
hidetofalseif you want thePopoverContentto be visible by default when there are pending transactions.- const [hide, setHide] = useState(true); + const [hide, setHide] = useState(false);spaceward/src/features/assets/queries.ts (3)
202-202: Consider using 'tokenSymbol' directly for clarityBy renaming
tokenSymboltotoken, it might introduce confusion iftokenis used elsewhere to refer to a different concept. UsingtokenSymboldirectly can enhance code readability and prevent potential misunderstandings.
700-704: Safely accessendpoint.urlwhenendpointis not a stringWhen assigning
rpcEndpoint, ensure thatendpointindeed has aurlproperty when it's not a string to avoid potential undefined errors. Adding a type guard or additional checks can enhance the robustness of this code.You might consider:
rpcEndpoint = typeof endpoint === "string" ? endpoint : endpoint.url ?? "";
719-720: Confirm thatrpcClientscache updates are thread-safeUpdating
rpcClients[chainName]with{ client, rpcEndpoint }improves performance by caching clients. However, ifqueryCosmosClientscan be called concurrently, consider using synchronization mechanisms to prevent race conditions when writing torpcClients.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (10)
- spaceward/src/components/ui/popover.tsx (1 hunks)
- spaceward/src/config/tokens.ts (1 hunks)
- spaceward/src/features/actions/StatusSidebar.tsx (8 hunks)
- spaceward/src/features/actions/hooks.ts (1 hunks)
- spaceward/src/features/actions/util.ts (2 hunks)
- spaceward/src/features/assets/queries.ts (6 hunks)
- spaceward/src/features/modals/SendAssets.tsx (3 hunks)
- spaceward/src/hooks/state.tsx (2 hunks)
- spaceward/src/lib/eth/constants.ts (2 hunks)
- spaceward/src/lib/eth/util.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- spaceward/src/features/actions/hooks.ts
Additional comments not posted (14)
spaceward/src/components/ui/popover.tsx (1)
14-14: LGTM: Improved flexibility for Portal mountingThe addition of
forceMount={props.forceMount}to thePopoverPrimitive.Portalcomponent is a good enhancement. This change allows users of thePopoverContentcomponent to control the mounting behavior of the Portal, which can be useful in scenarios where you need to ensure the Portal is always mounted or for better control over animations and transitions.spaceward/src/lib/eth/constants.ts (2)
19-19: LGTM! The BSC token information is accurate and consistent.The update to the
tokenproperty for Binance Smart Chain (BSC) is consistent with the new type definition and provides accurate information about the BNB token.
23-24: LGTM! The Ethereum Classic configuration is accurate and complete.The updates to the Ethereum Classic (ETC) configuration are consistent with the new type definition. The
tokenproperty provides accurate information about the ETC token, and the addition of thetitleproperty improves clarity.spaceward/src/lib/eth/util.ts (1)
3-3: LGTM: New import added for the capitalize function.The import of the
capitalizefunction is correctly added and will be used in thegetProviderfunction.spaceward/src/hooks/state.tsx (3)
105-105: Improved data synchronizationThe direct assignment of
datatoref.currentsimplifies the code and ensures immediate synchronization. This change eliminates the need for auseEffecthook, reducing complexity and potential timing issues.
120-127: Improved code formattingThe adjustments to the indentation in the JSON serialization logic enhance code readability without altering the functionality. This change aligns with best practices for code formatting.
Line range hint
105-127: Overall improvements to code clarity and structureThe changes in this file enhance code readability and maintain existing functionality. The removal of the
useEffecthook simplifies the data synchronization process, while the formatting adjustments improve overall code structure. These modifications align with best practices and should contribute to easier maintenance of the codebase.spaceward/src/config/tokens.ts (1)
219-223: LGTM. Consider utilizing the newtitleproperty.The addition of the optional
titleproperty to the_ENABLED_ETH_CHAINStype is a good improvement that allows for more descriptive information about each chain. This change is consistent with the PR objectives and doesn't affect existing functionality.However, I noticed that the
titleproperty is not currently used in the array initialization below. To fully utilize this change, consider the following:
- Update the
_ENABLED_ETH_CHAINSarray to includetitlefor each chain where appropriate.- Verify if other parts of the codebase need to be updated to use this new property.
To check for potential usage of the
titleproperty, you can run the following command:This will help identify if there are any existing references to a
titleproperty that this change is meant to support.spaceward/src/features/modals/SendAssets.tsx (2)
20-21: Approved: Necessary imports for React QueryThe import statements for
useQueryandqueryCosmosClientsare correctly added to utilize React Query for data fetching.
107-107: Verify: Ensure error handling covers all scenariosThrowing a generic error when
rpcis not found may not provide enough context. Verify that this error handling aligns with the application's error management strategy.Consider providing more informative feedback to the user or handling the error gracefully.
spaceward/src/features/actions/StatusSidebar.tsx (1)
417-417: Ensureitem.chainNameis defined before usingWhen displaying the status message,
item.chainNameis used without null or undefined checks. Ifitem.chainNameis undefined, it could result in displaying "Awaiting broadcast on undefined" or cause an error.Please confirm that
item.chainNameis always defined whenitem.statusisQueuedActionStatus.AwaitingBroadcast. If there's a possibility it could be undefined, consider providing a default value or adding a conditional check.spaceward/src/features/assets/queries.ts (3)
672-672: Confirm consistent use of the updatedclientsarrayThe
clientsarray now contains tuples of[CosmosQueryClient, string, string]. Ensure that all iterations and usages of this array unpack all three elements (client,chainName, andrpcEndpoint) to prevent potential runtime errors.You can verify this by searching for array destructuring patterns:
430-430: Verify all usages ofclientsare updated to the new tuple structureThe
clientsparameter has been updated to include an additional string in the tuple. Please ensure that all functions and variables that useclientsare updated accordingly to match the new structure[CosmosQueryClient, string, string][].Run the following script to check for any inconsistent usage of
clients:
632-635: Verify all usages ofrpcClientsare updated to the new structureThe
rpcClientsvariable now stores an object containing bothclientandrpcEndpoint. Please ensure that all references torpcClientsthroughout the codebase are updated to reflect this change by accessingclientandrpcEndpointaccordingly.Run the following script to find all usages of
rpcClientsthat may need updating:
| "1": { | ||
| rpc: ["https://cloudflare-eth.com", "https://eth.llamarpc.com"], | ||
| title: "Ethereum", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding the token property for consistency.
The addition of the title property improves clarity for the Ethereum chain. However, the token property is missing, which is inconsistent with other chain configurations and the type definition.
For consistency, consider adding the token property to the Ethereum configuration:
"1": {
rpc: ["https://cloudflare-eth.com", "https://eth.llamarpc.com"],
title: "Ethereum",
token: { symbol: "ETH", name: "Ether" },
},
spaceward/src/lib/eth/util.ts
Outdated
| @@ -53,7 +56,7 @@ export const getProvider = (type: SupportedNetwork) => { | |||
| } | |||
|
|
|||
| const provider = providers[type]; | |||
| return { provider, token }; | |||
| return { provider, tokenSymbol, tokenName, title }; | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update getProvider Usage Across the Codebase
The getProvider function now returns additional properties (tokenSymbol, tokenName, title) alongside provider. However, several calls to this function are only destructuring the provider, ignoring the new properties. Please update these instances to handle the expanded return structure.
Affected Files:
spaceward/src/features/actions/util.ts(Line 264)spaceward/src/features/assets/queries.ts(Lines 202, 286, 599)spaceward/src/features/home/TotalAssetValue.tsx(Line 12)spaceward/src/features/home/HomeAssets.tsx(Line 17)spaceward/src/features/assets/Assets.tsx(Line 18)spaceward/src/features/modals/util.ts(Line 75)spaceward/src/features/intents/util/query.ts(Line 3)spaceward/src/components/SendEth.tsx(Line 11)
Analysis chain
LGTM: Updated return value of getProvider function.
The function now returns an object with additional properties (tokenSymbol, tokenName, title) along with the provider. This change enhances the information provided by the function.
Please ensure that all calls to getProvider across the codebase have been updated to handle the new return object structure. Run the following script to identify potential places that need updating:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getProvider function
# Expected result: List of files and line numbers where getProvider is called
rg --type typescript -n 'getProvider\(' --glob '!spaceward/src/lib/eth/util.ts'
Length of output: 116
Script:
#!/bin/bash
# Description: Find all usages of getProvider function
# Expected result: List of files and line numbers where getProvider is called
rg -g '*.ts' -g '*.tsx' -n 'getProvider\(' --glob '!spaceward/src/lib/eth/util.ts'
Length of output: 1014
| rpcEndpoint, | ||
| }: { | ||
| action: QueuedAction; | ||
| w: IWeb3Wallet | null; | ||
| walletManager: WalletManager; | ||
| rpcEndpoint?: string; |
There was a problem hiding this comment.
Make rpcEndpoint a required parameter in handleCosmos
Since handleCosmos throws an error when rpcEndpoint is not provided, the rpcEndpoint parameter should be marked as required in the function's type definitions.
Apply this diff to make rpcEndpoint a required parameter:
export const handleCosmos = async ({
action,
w,
queryClient,
rpcEndpoint,
}: {
action: QueuedAction;
w: IWeb3Wallet | null;
- rpcEndpoint?: string;
+ rpcEndpoint: string;
queryClient: QueryClient;
}) => {Committable suggestion was skipped due to low confidence.
| const [, , rpc] = cosmosClients?.find((item) => { | ||
| return item[1] === chainName | ||
| }) ?? []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Suggestion: Improve clarity when extracting rpc
Destructuring array elements by index reduces code readability and can be error-prone. Consider refactoring to make the code clearer.
Refactored code:
- const [, , rpc] = cosmosClients?.find((item) => {
- return item[1] === chainName
- }) ?? [];
+ const client = cosmosClients?.find((item) => item[1] === chainName);
+ const rpc = client ? client[2] : undefined;Alternatively, if cosmosClients is an array of objects:
+ const client = cosmosClients?.find((item) => item.chainName === chainName);
+ const rpc = client?.rpc;This refactor enhances readability by explicitly accessing the rpc value.
Committable suggestion was skipped due to low confidence.
| @@ -31,6 +32,7 @@ | |||
|
|
|||
| const { spaceId } = useSpaceId(); | |||
| const { queryBalances } = useAssetQueries(spaceId); | |||
| const cosmosClients = useQuery(queryCosmosClients(walletManager)).data; | |||
There was a problem hiding this comment.
Issue: Handle loading and error states for useQuery
The cosmosClients data fetched via useQuery may be undefined while loading or if an error occurs. Accessing cosmosClients without checking can lead to runtime errors.
Consider destructuring the result of useQuery to handle isLoading and error, ensuring that cosmosClients is defined before usage:
- const cosmosClients = useQuery(queryCosmosClients(walletManager)).data;
+ const { data: cosmosClients, isLoading, error } = useQuery(queryCosmosClients(walletManager));
+ if (isLoading) {
+ // Optionally handle the loading state, e.g., disable submit button
+ }
+ if (error) {
+ // Handle the error state
+ console.error(error);
+ }Ensure that cosmosClients is checked for availability before it's used elsewhere in the component.
Committable suggestion was skipped due to low confidence.
| const [, , rpcEndpoint] = clientsRef.current?.find((v) => v[1] === item?.chainName) ?? []; | ||
| res = await handleCosmos({ action: item, w, queryClient, rpcEndpoint }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fragile array destructuring when extracting rpcEndpoint
Destructuring with positional indexes can lead to maintenance issues if the structure of the array changes. Relying on specific indices makes the code less readable and more error-prone.
Suggested refactor:
Consider restructuring your data to use objects instead of arrays, allowing for property-based access which is more robust.
If modifying the data structure is not feasible, you can enhance readability by destructuring with meaningful variable names:
- const [, , rpcEndpoint] = cosmosClients?.find((v) => v[1] === item?.chainName) ?? [];
+ const clientEntry = cosmosClients?.find((v) => v[1] === item?.chainName) ?? [];
+ const rpcEndpoint = clientEntry ? clientEntry[2] : undefined;Or, if possible, adjust the data to be an array of objects:
// Example structure
[
{ key: ..., chainName: ..., rpcEndpoint: ... },
// ...
]And then use:
const clientEntry = cosmosClients?.find((v) => v.chainName === item?.chainName);
const rpcEndpoint = clientEntry?.rpcEndpoint;This makes the code clearer and less dependent on the array structure.
| <Popover onOpenChange={open => { | ||
| setHide(!open); | ||
| }}> |
There was a problem hiding this comment.
Inconsistent visibility logic for PopoverContent
The logic controlling the visibility and mounting of PopoverContent may lead to unintended behavior:
- You are toggling
hidebased on theopenstate withsetHide(!open). - The
classNameapplies thehiddenclass whenhideistrue. - The
forceMountprop is set tohide ? true : undefined, which forces mounting whenhideistrue.
This could result in the PopoverContent being mounted and hidden when it's supposed to be visible, and vice versa.
Suggested fix:
Simplify the state management by using the open state directly and adjust the forceMount logic accordingly.
- const [hide, setHide] = useState(true);
+ const [open, setOpen] = useState(false);
...
<Popover
- onOpenChange={open => {
- setHide(!open);
- }}
+ open={open}
+ onOpenChange={setOpen}
>Update the PopoverContent:
<PopoverContent
side="left"
sideOffset={20}
- className={clsx("p-0", { hidden: hide })}
- forceMount={hide ? true : undefined}
+ className="p-0"
+ forceMount
>This ensures that:
- The
openstate directly controls the visibility. forceMountis alwaystrueto keep the content in the DOM.- The
hiddenclass is no longer needed since the visibility is managed by thePopovercomponent.
Also applies to: 497-498
| @@ -46,6 +48,10 @@ | |||
| function ActionItem({ single, ...item }: ItemProps) { | |||
| const queryClient = useQueryClient(); | |||
| const { walletManager } = useContext(walletContext); | |||
| const cosmosClients = useQuery(queryCosmosClients(walletManager)).data; | |||
There was a problem hiding this comment.
Handle loading and error states from useQuery
The cosmosClients variable is directly accessed from useQuery(queryCosmosClients(walletManager)).data without checking for loading or error states. This might lead to issues if the data is not immediately available.
Suggested fix:
Destructure the result of useQuery to handle loading and error states:
const { data: cosmosClients, isLoading, error } = useQuery(queryCosmosClients(walletManager));
if (isLoading) {
// Handle loading state, e.g., return a loader or null
}
if (error) {
// Handle error state, e.g., display an error message
}This ensures that your component handles all possible states and doesn't attempt to access data that may be undefined.
| const cosmosClients = useQuery(queryCosmosClients(walletManager)).data; | ||
| const clientsRef = useRef(cosmosClients); | ||
| clientsRef.current = cosmosClients; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unnecessary use of useRef for cosmosClients
The clientsRef is initialized with cosmosClients and then updated on each render. Since cosmosClients is already reactive, using a ref here is unnecessary and can be simplified.
Suggested refactor:
Remove the useRef and use cosmosClients directly in your code.
- const clientsRef = useRef(cosmosClients);
- clientsRef.current = cosmosClients;And update the usage accordingly:
- const [, , rpcEndpoint] = clientsRef.current?.find((v) => v[1] === item?.chainName) ?? [];
+ const [, , rpcEndpoint] = cosmosClients?.find((v) => v[1] === item?.chainName) ?? [];This simplifies the code and avoids unnecessary complexity.
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 cosmosClients = useQuery(queryCosmosClients(walletManager)).data; | |
| const clientsRef = useRef(cosmosClients); | |
| clientsRef.current = cosmosClients; | |
| const cosmosClients = useQuery(queryCosmosClients(walletManager)).data; |
| const retry = (rpcRetry[chainName] ?? 0) % (retries + 1); | ||
| rpcRetry[chainName] = retry + 1; | ||
|
|
||
| if (!client) { | ||
| if (!client || !rpcEndpoint) { |
There was a problem hiding this comment.
Ensure proper handling when rpcEndpoint is undefined
When checking if (!client || !rpcEndpoint), consider scenarios where client is defined but rpcEndpoint is undefined. Verify that the logic accounts for cases where only one of these variables is missing, and that subsequent code handles these situations appropriately.
Consider adjusting the condition to explicitly handle each case or adding further validation as needed.
Summary by CodeRabbit
New Features
Popovercomponent behavior with conditional mounting.Bug Fixes
SendAssetsModal.Documentation
QueuedActionStatusenum.Refactor