[client] show all my trades in history #2046
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
You are out of MentatBot reviews. Your usage will refresh November 11 at 08:00 AM. |
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
client/src/ui/components/trading/TradeHistoryEvent.tsx (2)
37-37: LGTM! Crown icon removal aligns with showing all trades.The removal of the Crown icon is appropriate since trades will now be shown for both maker and taker roles.
Consider improving the trade direction display to be more explicit about the user's role. For example:
- <div className={`text-sm my-auto flex flex-row items-center justify-start`}>{taker}</div> + <div className={`text-sm my-auto flex flex-row items-center justify-start`}> + {`${taker} ${trade.event.isYours ? '(You)' : ''}`} + </div>
Line range hint
38-45: Consider clarifying trade direction based on user's role.The trade description always shows "bought" from taker's perspective, which might be confusing when viewing trades where the user was the maker.
Consider adjusting the trade description to be clearer:
<div className="text-sm my-auto flex flex-row"> - <div>{"bought"}</div> + <div>{trade.event.isYours ? (trade.event.takerId === currentUser ? "bought" : "sold") : "bought"}</div> <ResourceIcon resource={ResourcesIds[Number(trade.event.resourceTaken.resourceId)]} size={"sm"} /> <div>{`${currencyIntlFormat(divideByPrecision(trade.event.resourceTaken.amount), 2)} for ${currencyIntlFormat( divideByPrecision(trade.event.resourceGiven.amount), 2, )}`}</div> <ResourceIcon resource={ResourcesIds[Number(trade.event.resourceGiven.resourceId)]} size={"sm"} /> </div>client/src/ui/components/trading/MarketTradingHistory.tsx (2)
64-64: LGTM! Trade ownership check now includes both maker and taker roles.The condition correctly identifies trades where the user is either the maker or taker, addressing the original issue of showing trades from only one side.
Consider extracting the ownership check into a more readable form:
-isYours: takerOwner.address === BigInt(address) || makerOwner.address === BigInt(address), +isYours: [takerOwner.address, makerOwner.address].includes(BigInt(address)),
Line range hint
7-7: Consider pagination for improved scalability.Now that the trade history includes both maker and taker trades, users might see up to twice as many trades. While the current MAX_TRADES limit of 100 helps manage performance, consider implementing pagination for better scalability and user experience.
Would you like me to propose a pagination implementation that maintains the current filtering capabilities?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/src/ui/components/trading/MarketTradingHistory.tsx(2 hunks)client/src/ui/components/trading/TradeHistoryEvent.tsx(1 hunks)
🔇 Additional comments (2)
client/src/ui/components/trading/TradeHistoryEvent.tsx (1)
Line range hint 1-67: Verify the trade event interface implementation.
Let's check how the isYours property is implemented in MarketTradingHistory to ensure it correctly identifies both maker and taker roles.
client/src/ui/components/trading/MarketTradingHistory.tsx (1)
51-53: LGTM! Maker owner retrieval is properly implemented.
The addition of maker owner retrieval and validation follows the same pattern as the taker owner, ensuring consistent handling of both sides of trades.
Let's verify that the maker_id is always available in AcceptOrder events:
✅ Verification successful
Maker ID retrieval is properly implemented and verified
Based on the code analysis:
- The
maker_idis a required field in the AcceptOrder props interface (from sdk/packages/eternum/src/types/provider.ts) - The component properly retrieves both maker and taker owners using consistent patterns
- The implementation correctly handles the PR objective of showing both sides of trades
- The early return prevents undefined behavior when maker owner is not found
The verification confirms that:
- The AcceptOrder event structure includes maker_id as shown in the SDK types
- The component properly handles maker_id retrieval and validation
- The implementation follows the same pattern as taker_id handling
- The changes successfully fulfill the requirement to show all trades in history
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AcceptOrder event definitions to confirm maker_id is a required field
ast-grep --pattern 'struct AcceptOrder {
$$$
maker_id: $_
$$$
}'
Length of output: 72
Script:
#!/bin/bash
# Let's try different approaches to find AcceptOrder event structure
# 1. Search for AcceptOrder in typescript/solidity files
rg -t ts -t sol "AcceptOrder" -A 5 -B 5
# 2. Search for maker_id usage in the context of events
rg "maker_id.*event" -t ts
# 3. Look for the specific file where events are defined
fd "events?.ts" --type f
Length of output: 223
Script:
#!/bin/bash
# Let's check the content of the identified files and their imports
cat client/src/hooks/helpers/useBattleEvents.tsx
cat client/src/ui/components/trading/TradeHistoryEvent.tsx
# Also search for any type definitions related to AcceptOrder
rg -t ts "type.*AcceptOrder" -A 3
rg -t ts "interface.*AcceptOrder" -A 3
Length of output: 6426
Script:
#!/bin/bash
# Let's check the MarketTradingHistory.tsx file to understand the full context
cat client/src/ui/components/trading/MarketTradingHistory.tsx
# Also check for any event type definitions in the codebase
rg -t ts "AcceptOrder.*event" -B 3 -A 3
Length of output: 5317
Fixes #2041
Summary by CodeRabbit
New Features
MarketTradingHistorycomponent to accurately reflect trades involving the current user as both maker and taker.Bug Fixes
TradeHistoryEventcomponent by removing the conditional display of theCrownicon next to the taker name.