feat: trade history - Githubissues

edisontim commented 5 days ago

PR Type

Enhancement, Bug fix

Description

Exported client constant in eventPoller.tsx.
Added AddressName component and address-related functions in useEntities.tsx.
Added DetachedResource component and getResourceBalance function in useResources.tsx.
Cleaned up commented-out address fetching code in useAddressStore.tsx.
Reorganized imports in several components for better readability.
Implemented trade history querying and display in MarketTradingHistory.tsx.
Added new TradeHistoryEvent component.
Updated battle view after battle action in BattleActions.tsx.
Removed unused hexToAscii function in utils.tsx.
Added constants for accept and cancel order events in events.ts.
Updated armies tick interval to 30 seconds in global.ts.
Commented out .env.local copy command in client.sh.
Updated block number and class hashes in manifest.json and manifest.toml.
Added new events for AcceptOrder and CancelOrder in trade_systems.cairo.

Changes walkthrough 📝

Relevant files

Enhancement

17 files

eventPoller.tsx Export `client` constant for external usage. client/src/hooks/eventPoller.tsx - Exported `client` constant.	+1/-1
useEntities.tsx `Add address-related functions and components.` client/src/hooks/helpers/useEntities.tsx Added `AddressName` to the components. Implemented `getPlayerAddressFromEntity` and `getAddressNameFromEntity` functions.	+30/-1
useResources.tsx `Add DetachedResource component and getResourceBalance function.` client/src/hooks/helpers/useResources.tsx Added `DetachedResource` to the components. Implemented `getResourceBalance` function.	+12/-15
useAddressStore.tsx `Clean up commented-out address fetching code.` client/src/hooks/store/useAddressStore.tsx - Removed commented-out code related to fetching address name.	+0/-17
BankList.tsx `Reorganize imports in BankList component.` client/src/ui/components/bank/BankList.tsx - Reorganized imports for better readability.	+5/-7
Login.tsx `Clean up imports and remove debug logs.` client/src/ui/components/chat/Login.tsx Moved `useMemo` and `useState` imports for consistency. Removed console.log statements.	+1/-5
MarketModal.tsx `Reorganize imports and update MarketTradingHistory usage.` client/src/ui/components/trading/MarketModal.tsx Reorganized imports for better readability. Updated `MarketTradingHistory` component usage.	+12/-12
MarketTradingHistory.tsx `Implement trade history querying and display.` client/src/ui/components/trading/MarketTradingHistory.tsx - Implemented trade history querying and display.	+114/-14
TradeHistoryEvent.tsx `Add TradeHistoryEvent component.` client/src/ui/components/trading/TradeHistoryEvent.tsx - Added new component for displaying trade history events.	+87/-0
BattleActions.tsx `Update battle view after battle action.` client/src/ui/modules/military/battle-view/BattleActions.tsx - Added `setBattleView` call after battle action.	+2/-1
utils.tsx `Remove unused hexToAscii function.` client/src/ui/utils/utils.tsx - Removed unused `hexToAscii` function.	+1/-13
events.ts `Add constants for accept and cancel order events.` sdk/packages/eternum/src/constants/events.ts Added new constants for `ACCEPT_ORDER_EVENT` and `CANCEL_ORDER_EVENT`.	+2/-0
global.ts `Update armies tick interval to 30 seconds.` sdk/packages/eternum/src/constants/global.ts - Updated `armiesTickIntervalInSeconds` to 30 seconds.	+1/-1
client.sh `Comment out .env.local copy command.` scripts/client.sh - Commented out the line copying `.env.sample` to `.env.local`.	+1/-1
manifest.json `Update block number and class hashes.` contracts/manifests/dev/manifest.json - Updated block number and class hashes.	+3/-3
manifest.toml `Update block number and class hashes.` contracts/manifests/dev/manifest.toml - Updated block number and class hashes.	+3/-3
trade_systems.cairo `Add AcceptOrder and CancelOrder events with timestamps.` contracts/src/systems/trade/contracts/trade_systems.cairo Added new events for `AcceptOrder` and `CancelOrder`. Updated `CreateOrder` event to include timestamp.	+44/-3

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

vercel[bot] commented 5 days ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
eternum	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 29, 2024 8:51am

github-actions[bot] commented 5 days ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The `useEntities` hook in `useEntities.tsx` has added complex nested functions like `getPlayerAddressFromEntity` and `getAddressNameFromEntity`. These functions could potentially lead to performance issues due to deep nested calls and should be optimized or simplified.
	Code Organization: The `useResources.tsx` file has undergone significant refactoring with the addition of `getResourceBalance` and changes to import statements. It's crucial to ensure that these changes do not affect existing functionalities and that they follow best practices for hooks.
	Data Handling: The new `MarketTradingHistory.tsx` component involves complex GraphQL queries and state management which could be prone to errors. It's important to ensure that the data handling is robust, especially in the `queryTrades` function where multiple states are updated based on query results.

github-actions[bot] commented 5 days ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Provide a fallback URL for `VITE_TORII_GRAPHQL` to avoid potential runtime errors ___ Since `import.meta.env.VITE_TORII_GRAPHQL` might be undefined, it is safer to provide a fallback URL or handle the undefined case to avoid runtime errors. [client/src/hooks/eventPoller.tsx [5]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-5cca517b2372b7dc7cf8c087f3686e8faf4efc3a0334cad451bb62b41d4f7cc6R5-R5) ```diff -export const client = new GraphQLClient(import.meta.env.VITE_TORII_GRAPHQL!); +export const client = new GraphQLClient(import.meta.env.VITE_TORII_GRAPHQL \|\| 'http://default-url.com'); ``` Suggestion importance[1-10]: 9 Why: Providing a fallback URL is crucial to avoid runtime errors if `import.meta.env.VITE_TORII_GRAPHQL` is undefined, making the code more resilient.	9
	Ensure `class_hash` and `original_class_hash` are correctly assigned distinct values if needed ___ Verify that the `class_hash` and `original_class_hash` values are intended to be the same, as this could be an error or oversight. Typically, these should differ if a new version of a contract is being deployed. [contracts/manifests/dev/manifest.toml [220-221]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-43499a9633d551acabe80100dc29d8995890cd62b12169122658233caea498b3R220-R221) ```diff class_hash = "0x6d3b153c4ca68394dc91fb4f0ab8c7dd4f6c6a6e9f80d64a5d03c039a4afa04" -original_class_hash = "0x6d3b153c4ca68394dc91fb4f0ab8c7dd4f6c6a6e9f80d64a5d03c039a4afa04" +original_class_hash = "" ``` Suggestion importance[1-10]: 9 Why: This suggestion highlights a critical aspect of contract versioning. Typically, `class_hash` and `original_class_hash` should differ to reflect updates, and ensuring this can prevent potential deployment issues.	9
	Update the `block_number` to a realistic value ___ Ensure that the `block_number` is updated to a realistic and contextually appropriate value, rather than a hardcoded small number like 19, which might be a placeholder or error. [contracts/manifests/dev/manifest.toml [8]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-43499a9633d551acabe80100dc29d8995890cd62b12169122658233caea498b3R8-R8) ```diff -block_number = 19 +block_number = ``` Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential issue where a placeholder value might have been used. Ensuring the `block_number` is realistic is crucial for maintaining accurate state and data integrity.	8
Possible bug	Add null checks for `account.address` before using it in `BigInt` conversion ___ Consider checking for undefined or null values for `account.address` before converting it to `BigInt` to avoid potential runtime errors if `account.address` is not set. [client/src/hooks/helpers/useEntities.tsx [33-34]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-655bd94eaba19bfcec06d7b736d2e64944c31d55f2b38819201a2b0b612787e4R33-R34) ```diff -const otherRealms = useEntityQuery([Has(Realm), NotValue(Owner, { address: BigInt(account.address) })]); -const playerStructures = useEntityQuery([Has(Structure), HasValue(Owner, { address: BigInt(account.address) })]); +const otherRealms = account.address ? useEntityQuery([Has(Realm), NotValue(Owner, { address: BigInt(account.address) })]) : []; +const playerStructures = account.address ? useEntityQuery([Has(Structure), HasValue(Owner, { address: BigInt(account.address) })]) : []; ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential runtime error by ensuring `account.address` is defined before converting it to `BigInt`. This is a significant improvement for code robustness.	8
Enhancement	Add a check for empty results in `getResourceBalance` to handle no entities found ___ To ensure that `getResourceBalance` function handles cases where `runQuery` might return an empty array, add a check to return an empty array or a default value when no entities are found. [client/src/hooks/helpers/useResources.tsx [168-170]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-da868a5a06b096d28f8b0e970a0260a8d26d34f5acfafa3dcbd0156da8f15749R168-R170) ```diff const getResourceBalance = (entityId: bigint) => { const detachedResourceEntityIds = runQuery([HasValue(DetachedResource, { entity_id: entityId })]); + if (!detachedResourceEntityIds.length) return []; return Array.from(detachedResourceEntityIds).map((entityId) => getComponentValue(DetachedResource, entityId)); }; ``` Suggestion importance[1-10]: 7 Why: This enhancement ensures that the function handles cases where no entities are found, preventing potential errors and improving code reliability.	7
	Add validation before emitting trade events to ensure data consistency ___ Consider adding error handling or validation logic when emitting `AcceptOrder` and `CancelOrder` events to ensure that the state changes are valid and consistent before broadcasting these events. [contracts/src/systems/trade/contracts/trade_systems.cairo [282-287]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-e84e5e5d21898c44ad2cd3739a38f25dc14b6b8f1b627db54326aabdb5bcc22dR282-R287) ```diff -emit!( - world, - (AcceptOrder { - taker_id, - maker_id: trade.maker_id, - trade_id, - timestamp: starknet::get_block_timestamp() - }) -) +if validate_trade(trade_id) { + emit!( + world, + (AcceptOrder { + taker_id, + maker_id: trade.maker_id, + trade_id, + timestamp: starknet::get_block_timestamp() + }) + ) +} ``` Suggestion importance[1-10]: 7 Why: Adding validation logic before emitting events can enhance the robustness of the system by ensuring state changes are valid. However, the exact implementation of `validate_trade` is not provided, which slightly reduces the score.	7
	Use a trade-specific timestamp for the `CancelOrder` event to reflect the actual event time ___ Ensure that the timestamp used in the `CancelOrder` event is consistent with the trade's lifecycle, possibly using a trade-specific timestamp rather than the current block timestamp. [contracts/src/systems/trade/contracts/trade_systems.cairo [316-321]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-e84e5e5d21898c44ad2cd3739a38f25dc14b6b8f1b627db54326aabdb5bcc22dR316-R321) ```diff emit!( world, (CancelOrder { taker_id: trade.taker_id, maker_id: trade.maker_id, trade_id, - timestamp: starknet::get_block_timestamp() + timestamp: trade.creation_timestamp # Assuming `creation_timestamp` is available }) ) ``` Suggestion importance[1-10]: 6 Why: Using a trade-specific timestamp can improve the accuracy of event logging. However, the suggestion assumes the existence of a `creation_timestamp`, which may not be available or appropriate in all contexts.	6
Performance	Optimize GraphQL queries in `queryTrades` by using a single query with variables ___ To improve the performance of the `queryTrades` function, consider using a single GraphQL query with variables instead of multiple queries for each event type. [client/src/ui/components/trading/MarketTradingHistory.tsx [26-56]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-7bd056914b735bd4e71bffbf268e8cc14e71fa83a6c9902f3ed1bfd05e7e061fR26-R56) ```diff const trades: any = await client.request(` - query { - createdOrders: events(keys: ["${CREATE_ORDER_EVENT}", ""]) { - edges { - node { - id - keys - data - } - } - } - acceptOrders: events(keys: ["${ACCEPT_ORDER_EVENT}", ""]) { - edges { - node { - id - keys - data - } - } - } - cancelOrders: events(keys: ["${CANCEL_ORDER_EVENT}", ""]) { + query GetOrders($eventKeys: [String!]!) { + orders: events(keys: $eventKeys) { edges { node { id keys data } } } } -`); +`, { + eventKeys: [CREATE_ORDER_EVENT, ACCEPT_ORDER_EVENT, CANCEL_ORDER_EVENT, ""] +}); ``` Suggestion importance[1-10]: 6 Why: This suggestion improves performance by consolidating multiple GraphQL queries into a single query with variables, reducing the number of network requests.	6

Category	Suggestion	Score
Possible issue	Provide a fallback URL for `VITE_TORII_GRAPHQL` to avoid potential runtime errors ___ Since `import.meta.env.VITE_TORII_GRAPHQL` might be undefined, it is safer to provide a fallback URL or handle the undefined case to avoid runtime errors. [client/src/hooks/eventPoller.tsx [5]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-5cca517b2372b7dc7cf8c087f3686e8faf4efc3a0334cad451bb62b41d4f7cc6R5-R5) ```diff -export const client = new GraphQLClient(import.meta.env.VITE_TORII_GRAPHQL!); +export const client = new GraphQLClient(import.meta.env.VITE_TORII_GRAPHQL \|\| 'http://default-url.com'); ``` Suggestion importance[1-10]: 9 Why: Providing a fallback URL is crucial to avoid runtime errors if `import.meta.env.VITE_TORII_GRAPHQL` is undefined, making the code more resilient.	9
	Ensure `class_hash` and `original_class_hash` are correctly assigned distinct values if needed ___ Verify that the `class_hash` and `original_class_hash` values are intended to be the same, as this could be an error or oversight. Typically, these should differ if a new version of a contract is being deployed. [contracts/manifests/dev/manifest.toml [220-221]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-43499a9633d551acabe80100dc29d8995890cd62b12169122658233caea498b3R220-R221) ```diff class_hash = "0x6d3b153c4ca68394dc91fb4f0ab8c7dd4f6c6a6e9f80d64a5d03c039a4afa04" -original_class_hash = "0x6d3b153c4ca68394dc91fb4f0ab8c7dd4f6c6a6e9f80d64a5d03c039a4afa04" +original_class_hash = "" ``` Suggestion importance[1-10]: 9 Why: This suggestion highlights a critical aspect of contract versioning. Typically, `class_hash` and `original_class_hash` should differ to reflect updates, and ensuring this can prevent potential deployment issues.	9
	Update the `block_number` to a realistic value ___ Ensure that the `block_number` is updated to a realistic and contextually appropriate value, rather than a hardcoded small number like 19, which might be a placeholder or error. [contracts/manifests/dev/manifest.toml [8]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-43499a9633d551acabe80100dc29d8995890cd62b12169122658233caea498b3R8-R8) ```diff -block_number = 19 +block_number = ``` Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential issue where a placeholder value might have been used. Ensuring the `block_number` is realistic is crucial for maintaining accurate state and data integrity.	8
Possible bug	Add null checks for `account.address` before using it in `BigInt` conversion ___ Consider checking for undefined or null values for `account.address` before converting it to `BigInt` to avoid potential runtime errors if `account.address` is not set. [client/src/hooks/helpers/useEntities.tsx [33-34]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-655bd94eaba19bfcec06d7b736d2e64944c31d55f2b38819201a2b0b612787e4R33-R34) ```diff -const otherRealms = useEntityQuery([Has(Realm), NotValue(Owner, { address: BigInt(account.address) })]); -const playerStructures = useEntityQuery([Has(Structure), HasValue(Owner, { address: BigInt(account.address) })]); +const otherRealms = account.address ? useEntityQuery([Has(Realm), NotValue(Owner, { address: BigInt(account.address) })]) : []; +const playerStructures = account.address ? useEntityQuery([Has(Structure), HasValue(Owner, { address: BigInt(account.address) })]) : []; ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential runtime error by ensuring `account.address` is defined before converting it to `BigInt`. This is a significant improvement for code robustness.	8
Enhancement	Add a check for empty results in `getResourceBalance` to handle no entities found ___ To ensure that `getResourceBalance` function handles cases where `runQuery` might return an empty array, add a check to return an empty array or a default value when no entities are found. [client/src/hooks/helpers/useResources.tsx [168-170]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-da868a5a06b096d28f8b0e970a0260a8d26d34f5acfafa3dcbd0156da8f15749R168-R170) ```diff const getResourceBalance = (entityId: bigint) => { const detachedResourceEntityIds = runQuery([HasValue(DetachedResource, { entity_id: entityId })]); + if (!detachedResourceEntityIds.length) return []; return Array.from(detachedResourceEntityIds).map((entityId) => getComponentValue(DetachedResource, entityId)); }; ``` Suggestion importance[1-10]: 7 Why: This enhancement ensures that the function handles cases where no entities are found, preventing potential errors and improving code reliability.	7
	Add validation before emitting trade events to ensure data consistency ___ Consider adding error handling or validation logic when emitting `AcceptOrder` and `CancelOrder` events to ensure that the state changes are valid and consistent before broadcasting these events. [contracts/src/systems/trade/contracts/trade_systems.cairo [282-287]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-e84e5e5d21898c44ad2cd3739a38f25dc14b6b8f1b627db54326aabdb5bcc22dR282-R287) ```diff -emit!( - world, - (AcceptOrder { - taker_id, - maker_id: trade.maker_id, - trade_id, - timestamp: starknet::get_block_timestamp() - }) -) +if validate_trade(trade_id) { + emit!( + world, + (AcceptOrder { + taker_id, + maker_id: trade.maker_id, + trade_id, + timestamp: starknet::get_block_timestamp() + }) + ) +} ``` Suggestion importance[1-10]: 7 Why: Adding validation logic before emitting events can enhance the robustness of the system by ensuring state changes are valid. However, the exact implementation of `validate_trade` is not provided, which slightly reduces the score.	7
	Use a trade-specific timestamp for the `CancelOrder` event to reflect the actual event time ___ Ensure that the timestamp used in the `CancelOrder` event is consistent with the trade's lifecycle, possibly using a trade-specific timestamp rather than the current block timestamp. [contracts/src/systems/trade/contracts/trade_systems.cairo [316-321]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-e84e5e5d21898c44ad2cd3739a38f25dc14b6b8f1b627db54326aabdb5bcc22dR316-R321) ```diff emit!( world, (CancelOrder { taker_id: trade.taker_id, maker_id: trade.maker_id, trade_id, - timestamp: starknet::get_block_timestamp() + timestamp: trade.creation_timestamp # Assuming `creation_timestamp` is available }) ) ``` Suggestion importance[1-10]: 6 Why: Using a trade-specific timestamp can improve the accuracy of event logging. However, the suggestion assumes the existence of a `creation_timestamp`, which may not be available or appropriate in all contexts.	6
Performance	Optimize GraphQL queries in `queryTrades` by using a single query with variables ___ To improve the performance of the `queryTrades` function, consider using a single GraphQL query with variables instead of multiple queries for each event type. [client/src/ui/components/trading/MarketTradingHistory.tsx [26-56]](https://github.com/BibliothecaDAO/eternum/pull/1043/files#diff-7bd056914b735bd4e71bffbf268e8cc14e71fa83a6c9902f3ed1bfd05e7e061fR26-R56) ```diff const trades: any = await client.request(` - query { - createdOrders: events(keys: ["${CREATE_ORDER_EVENT}", ""]) { - edges { - node { - id - keys - data - } - } - } - acceptOrders: events(keys: ["${ACCEPT_ORDER_EVENT}", ""]) { - edges { - node { - id - keys - data - } - } - } - cancelOrders: events(keys: ["${CANCEL_ORDER_EVENT}", ""]) { + query GetOrders($eventKeys: [String!]!) { + orders: events(keys: $eventKeys) { edges { node { id keys data } } } } -`); +`, { + eventKeys: [CREATE_ORDER_EVENT, ACCEPT_ORDER_EVENT, CANCEL_ORDER_EVENT, ""] +}); ``` Suggestion importance[1-10]: 6 Why: This suggestion improves performance by consolidating multiple GraphQL queries into a single query with variables, reducing the number of network requests.	6

BibliothecaDAO / eternum

feat: trade history #1043

PR Type

Description

Changes walkthrough 📝

PR Reviewer Guide 🔍

PR Code Suggestions ✨