fix: make events non-historical #2088
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request includes significant modifications to various event structures across multiple modules, primarily changing the historical tracking attribute from Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🔇 Additional comments (2)contracts/src/systems/combat/contracts/battle_systems.cairo (2)Line range hint The battle system implementation demonstrates:
The removal of
The removal of ✅ Verification successfulBased on the results, I can now generate the final response: Event handling architecture change is properly implemented The removal of
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any remaining references to EternumEvent and verify event handling patterns
# Test 1: Check for any remaining EternumEvent references
rg -l "EternumEvent"
# Test 2: Check event emission patterns to ensure consistency
rg -A 2 "emit_event"
# Test 3: Check event struct usage patterns
ast-grep --pattern 'struct $event_name {
$$$
event_id: EventType,
$$$
}'
Length of output: 9215 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 |
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look good and are very consistent. The PR removes the unused EternumEvent struct and updates all event structs to be non-historical by changing #[dojo::event(historical: true)] to #[dojo::event(historical: false)]. This appears to be a well-structured cleanup that will require an update to dojo.js. The changes are purely about event handling configuration and don't affect the actual event structures or their usage.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
contracts/src/models/event.cairo (1)
Line range hint
145-152: Consider enhancing guild event data structuresThe guild events (
CreateGuild,JoinGuild) capture minimal information. Consider adding:
- Guild metadata (description, rules)
- Member roles/permissions
- Guild hierarchy information
Example enhancement:
pub struct CreateGuild { #[key] guild_entity_id: ID, guild_name: felt252, + guild_description: felt252, + guild_rules: felt252, + creator_role: felt252, timestamp: u64 } pub struct JoinGuild { #[key] guild_entity_id: ID, #[key] address: ContractAddress, guild_name: felt252, + member_role: felt252, timestamp: u64 }Also applies to: 154-161
contracts/src/systems/transport/contracts/travel_systems.cairo (1)
36-36: Consider architectural implications of non-historical eventsThis change appears to be part of a broader architectural shift in event handling. Given that:
- Similar changes are being made across multiple modules
- There's a dependency on dojo.js PR bug: to small icon of the resource in the "my offers" list #329
Please ensure:
- The change aligns with dojo.js's new event handling architecture
- All related modules are consistently updated
- Documentation is updated to reflect the new event persistence model
Consider documenting the rationale behind making events non-historical and any new patterns for accessing recent event data in the system architecture documentation.
contracts/src/systems/map/contracts.cairo (1)
Line range hint
158-171: Add timestamp validation for event emissionsBoth
MapExploredandFragmentMineDiscoveredevents usestarknet::get_block_timestamp()directly. Consider adding validation to ensure the timestamp is within expected bounds and handling potential edge cases.Consider wrapping the timestamp retrieval in a validation function:
fn get_validated_timestamp() -> u64 { let timestamp = starknet::get_block_timestamp(); // Add validation logic here timestamp }Then update the event emissions to use this function:
-timestamp: starknet::get_block_timestamp() +timestamp: get_validated_timestamp()Also applies to: 211-220
contracts/src/systems/trade/contracts/trade_systems.cairo (2)
Line range hint
393-487: Refactor partial order creation logicThe implementation of partial order creation contains a large block of repeated code with multiple separator comments, suggesting this might be a temporary solution. This section could benefit from refactoring for better maintainability.
Consider extracting the order creation logic into a separate function:
+ fn create_new_partial_order( + ref world: WorldStorage, + trade: Trade, + maker_gives_resource_type: u8, + maker_gives_actual_amount: u128, + taker_gives_resource_type: u8, + taker_gives_actual_amount: u128, + maker_gives_resources_actual_weight: u128, + taker_gives_resources_actual_weight: u128 + ) -> ID { + let new_maker_gives_resources_felt_arr_id: ID = world.dispatcher.uuid(); + let new_taker_gives_resources_felt_arr_id: ID = world.dispatcher.uuid(); + + // Create detached resources + world.write_model( + @DetachedResource { + entity_id: new_maker_gives_resources_felt_arr_id, + index: 0, + resource_type: maker_gives_resource_type, + resource_amount: maker_gives_actual_amount + } + ); + // ... rest of the implementation + + let new_trade_id = world.dispatcher.uuid(); + world.write_model( + @Trade { + trade_id: new_trade_id, + maker_id: trade.maker_id, + // ... rest of the trade fields + } + ); + + new_trade_id + }This would make the code more maintainable and easier to test.
Line range hint
315-318: Improve error messages for better debuggingThe assertion messages could be more descriptive to aid in debugging:
Consider updating the error messages to be more specific:
- assert!(taker_gives_actual_amount.is_non_zero(), "amount taker gives must be greater than 0"); - assert!(maker_gives_resources.len() == 1, "only one resource type is supported for partial orders"); - assert!(maker_gives_resources.len() == taker_gives_resources.len(), "resources lengths must be equal"); + assert!(taker_gives_actual_amount.is_non_zero(), "Partial order: taker amount must be greater than 0"); + assert!(maker_gives_resources.len() == 1, "Partial order: only single resource type trades are supported"); + assert!(maker_gives_resources.len() == taker_gives_resources.len(), "Partial order: maker and taker resource lists must have equal length");contracts/src/systems/hyperstructure/contracts.cairo (2)
Line range hint
71-78: Consider audit implications of non-historical ownership changes.While current ownership is tracked in the Epoch model, losing historical ownership change events might impact audit capabilities. Consider if this history is required for:
- Dispute resolution
- Reward distribution calculations
- Compliance requirements
If audit trail is important, consider implementing an alternative tracking mechanism such as:
- Maintaining a separate ownership history model
- Using an external event indexer
- Implementing event archival in the application layer
Line range hint
94-99: Consider tracking game statistics separately.While changing to
historical: falsedoesn't affect core functionality, consider implementing a separate GameStatistics model if historical game results are needed for:
- Leaderboards
- Player achievements
- Season statistics
#[derive(Model, Drop, Serde)] struct GameStatistics { #[key] season: u64, winner_address: ContractAddress, winning_points: u128, end_timestamp: u64 }contracts/src/systems/combat/contracts/battle_systems.cairo (3)
Line range hint
928-932: Potential security issue with random number generation inbattle_pillageThe use of
random::choicesfor determining attack success may be susceptible to manipulation, as it relies on pseudo-randomness that can be influenced by validators. Consider integrating a secure source of randomness, such as verifiable random functions (VRFs), to prevent potential exploits and ensure fairness in the game mechanics.
Line range hint
941-947: Insecure random resource selection inbattle_pillageThe use of
random::choicesfor selecting resources to pillage may be vulnerable to manipulation. Attackers could potentially influence the randomness to favor certain outcomes. To enhance security, use a verifiable source of randomness or incorporate cryptographic techniques to ensure the integrity of the random selection process.
Line range hint
966-972: Security concern with random building destructionThe selection of buildings to destroy using
random::choicesmay be exploited if the randomness source is not secure. Consider adopting a secure random generation method to prevent potential manipulation by malicious actors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
contracts/src/models/event.cairo(8 hunks)contracts/src/systems/bank/contracts/liquidity.cairo(1 hunks)contracts/src/systems/bank/contracts/swap.cairo(1 hunks)contracts/src/systems/combat/contracts/battle_systems.cairo(2 hunks)contracts/src/systems/hyperstructure/contracts.cairo(4 hunks)contracts/src/systems/map/contracts.cairo(2 hunks)contracts/src/systems/resources/contracts/resource_systems.cairo(1 hunks)contracts/src/systems/trade/contracts/trade_systems.cairo(4 hunks)contracts/src/systems/transport/contracts/donkey_systems.cairo(1 hunks)contracts/src/systems/transport/contracts/travel_systems.cairo(1 hunks)
🔇 Additional comments (18)
contracts/src/models/event.cairo (3)
Line range hint 25-41: Verify battle event data completeness
The battle event structures (BattleStartData, BattleJoinData, BattleLeaveData, BattleClaimData, BattlePillageData) appear well-designed, but consider:
- Should battle events include a reference to the previous battle state?
- Are all necessary fields captured for battle reconstruction without historical events?
Let's check for potential data gaps:
Also applies to: 46-61, 64-79, 82-97, 100-119
✅ Verification successful
Based on the shell script results, I can now generate a final response since I have comprehensive information about the battle event system implementation:
Battle events provide complete state reconstruction capability
The battle event system is well-designed with complete state tracking:
- Each event type (
BattleStartData,BattleJoinData,BattleLeaveData,BattleClaimData,BattlePillageData) contains all necessary fields for state reconstruction:- Consistent entity IDs and references (
battle_entity_id,structure_entity_id, etc.) - Full participant tracking (attacker/defender/joiner/leaver details)
- Battle state fields (duration, location, resources)
- Historical linkage through event IDs
- Consistent entity IDs and references (
The events are properly consumed by both the client (React hooks, battle history components) and server (GraphQL schema, Discord bot) to maintain battle state without requiring historical events.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check battle state reconstruction logic
echo "Checking battle state management..."
ast-grep --pattern 'fn $_(battle: Battle)'
echo "Checking battle event consumers..."
rg -A 5 "BattleStartData|BattleJoinData|BattleLeaveData|BattleClaimData|BattlePillageData"
Length of output: 46623
25-25: Architectural Impact: Verify implications of non-historical events
The change from historical: true to historical: false across all events represents a significant architectural shift that could affect:
- Data persistence and queryability
- Historical analysis features
- Frontend functionality depending on historical event data
- Storage costs and performance
Let's verify the impact on existing code:
Also applies to: 46-46, 64-64, 82-82, 100-100, 122-122, 145-145, 154-154
✅ Verification successful
Historical events can be safely disabled for the current implementation
Based on the codebase analysis, the change from historical: true to historical: false appears safe because:
- Current event consumers (UI components like PillageHistory, discord bot) only process real-time events
- No evidence of historical data analysis or timeline features that require long-term event persistence
- Event queries in the client are primarily used for current state (e.g., minted realms count)
- Contract systems use events for immediate state transitions rather than historical lookups
The implementation shows events are mainly used for:
- Real-time UI updates through subscriptions
- Discord bot notifications
- Current state queries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for components that might depend on historical events
echo "Checking for historical event queries in frontend..."
rg -l "historical.*events?" --type ts
echo "Checking for event processing systems..."
rg -l "process.*event" --type cairo
echo "Checking for timeline or history features..."
rg -l "history|timeline" --type ts --type cairo
Length of output: 492
Script:
#!/bin/bash
# Let's try a more generic search without file type restrictions
echo "Checking for historical event references..."
rg -l "historical.*event"
echo "Checking for event processing..."
rg -l "process.*event"
echo "Checking for timeline/history features..."
rg -l "history|timeline"
# Check for any files importing or using these events
echo "Checking for imports or usage of event types..."
rg -l "use.*event::\{.*\}"
rg -l "from.*event import"
Length of output: 1061
Script:
#!/bin/bash
# Let's examine the specific files that use events to understand the impact
echo "Checking event usage in contracts..."
rg -A 5 "use.*event::" contracts/src/systems/realm/contracts.cairo contracts/src/systems/guild/contracts.cairo
echo "Checking PillageHistory implementation..."
rg -A 10 "PillageHistory" client/src/ui/components/military/PillageHistory.tsx
echo "Checking event processing in discord bot..."
rg -A 5 "process.*event" discord-bot/src/actors/parsed_event_processor.rs
# Check for event queries in the client
echo "Checking event queries in client..."
rg -A 5 "getEvents|queryEvents" client/src/
Length of output: 5807
Line range hint 122-142: Verify realm statistics tracking without historical events
The SettleRealmData structure captures comprehensive realm metrics, but with non-historical events:
- How will realm progression be tracked over time?
- How will resource production analytics be maintained?
Let's check for realm statistics dependencies:
✅ Verification successful
Non-historical events are appropriate for realm statistics tracking
Based on the codebase analysis, setting historical: false for SettleRealmData is correct because:
-
The realm statistics are tracked through multiple mechanisms:
- GraphQL schema shows comprehensive querying capabilities for
produced_resourceswith various filters (EQ, GT, LT, etc.) - The indexer captures and stores all realm events, making historical tracking possible at the application layer
- Resource production is packed efficiently using bit manipulation in
RealmResourcesImpl
- GraphQL schema shows comprehensive querying capabilities for
-
The event structure is used by multiple components:
- Discord bot for notifications
- Client-side hooks (
useRealm,useStructures) for real-time updates - Landing page queries for realm statistics
The non-historical event approach is more efficient as it avoids chain bloat while still maintaining traceability through the indexer and application layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check realm statistics usage
echo "Checking realm statistics consumers..."
ast-grep --pattern 'struct $_ {
$$$
produced_resources: $_
$$$
}'
echo "Checking realm analytics..."
rg -l "realm.*statistics|realm.*analytics"
Length of output: 270
Script:
#!/bin/bash
# Let's try a broader search for realm-related events and statistics tracking
# Check for realm-related events and their historical flags
echo "Checking realm-related events..."
ast-grep --pattern '#[dojo::event($_)]
pub struct $_ {
$$$
}'
# Search for realm data structures and systems
echo "Checking realm systems and data structures..."
rg -l "realm|Realm" --type cairo
# Look for resource tracking implementations
echo "Checking resource tracking..."
rg "produced_resources" -A 5 --type cairo
Length of output: 447
Script:
#!/bin/bash
# Let's try without file type restrictions
# Check for realm-related events and data structures
echo "Checking realm files and structures..."
rg -l "realm|Realm"
# Look for resource tracking and statistics
echo "Checking resource tracking..."
rg "produced_resources" -A 5
# Check for historical events or tracking
echo "Checking historical tracking..."
rg "historical.*true|statistics.*tracking" -A 5
Length of output: 27679
contracts/src/systems/transport/contracts/donkey_systems.cairo (1)
Line range hint 29-36: LGTM, but verify impact on existing queries.
The change from historical: true to historical: false aligns with the PR objective. However, since this event tracks important game mechanics like donkey burning and achievements, we should verify the impact.
Let's check for any existing queries or indexers that might rely on historical BurnDonkey events:
✅ Verification successful
Change to non-historical event is safe to proceed
The search results show that the BurnDonkey event is primarily used for UI display in the client (event details and type definitions) and contract component definitions. No historical querying or data processing dependencies were found that would be impacted by changing the event from historical to non-historical. The event is mainly used for:
- Real-time UI updates in
client/src/ui/modules/stream/eventDetails.tsx - Type definitions in the client
- Contract component definitions
- Test configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for queries or indexers that might be affected by the historical attribute change
# Look for GraphQL queries referencing BurnDonkey
rg -l "BurnDonkey.*historical|historical.*BurnDonkey" --type graphql
# Look for indexer configurations or processors
rg -l "BurnDonkey" --glob "*indexer*" --glob "*processor*"
# Look for test files that might need updating
rg -l "BurnDonkey" --glob "*test*" --glob "*spec*"
Length of output: 212
Script:
#!/bin/bash
# Let's try a broader search for BurnDonkey usage across the codebase
# and look for any query-related or data processing code
# Search for BurnDonkey references with context
rg -A 5 "BurnDonkey"
# Search for any files containing both "BurnDonkey" and "query" or "historical"
rg -l "BurnDonkey" | xargs rg -l "query|historical"
# Look for any GraphQL schema files or query files (with various extensions)
fd -e graphql -e gql -e ts -e js | xargs rg -l "BurnDonkey"
Length of output: 4931
contracts/src/systems/bank/contracts/liquidity.cairo (1)
34-34: Ensure compatibility with blocking PR
This change is blocked by dojo.js#329. Please verify:
- The required dojo.js version is specified in the project dependencies
- The changes align with any breaking changes in dojo.js
#!/bin/bash
# Check current dojo.js version in package.json
fd -t f "package.json" -x cat {} | jq '.dependencies["@dojoengine/dojo"]'
# Look for other historical events that might need similar updates
ast-grep --pattern $'#[dojo::event(historical: true)]'contracts/src/systems/bank/contracts/swap.cairo (2)
Line range hint 1-250: Implementation looks solid and well-structured
The swap system implementation demonstrates good practices:
- Clear separation of buy/sell logic
- Proper fee calculations and distribution
- Safe resource management
- Comprehensive event data capture
36-36: Verify implications of disabling historical event tracking
Changing historical to false for SwapEvent means these financial transaction records won't be preserved historically. This could impact:
- Transaction audit trails
- User dispute resolution capabilities
- Market analysis and reporting functionality
Consider implementing alternative data persistence mechanisms if historical swap data is required for:
- Regulatory compliance
- User transaction history
- Market analytics
contracts/src/systems/transport/contracts/travel_systems.cairo (1)
Line range hint 36-47: Verify impact of non-historical travel events
Changing travel events to non-historical could affect features that rely on historical travel data. Please ensure:
- No game mechanics depend on querying historical travel events
- No analytics or audit requirements need historical travel paths
- The change aligns with the broader system architecture
Let's check for potential dependencies on historical travel events:
✅ Verification successful
Based on the gathered evidence, I can now provide a final response:
Setting Travel events as non-historical aligns with system design
The change to make Travel events non-historical is consistent with the codebase's overall pattern:
- All other events in the system are also marked as
historical: false(as seen in resource_systems, trade_systems, map contracts, etc.) - No test files or client code show dependencies on historical travel events
- No evidence of features requiring historical travel data for audit or game mechanics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for code that might depend on historical travel events
# Check for references to travel events in queries or filters
rg -l "Travel.*historical|historical.*Travel"
# Look for potential dependencies in test files
rg -l "Travel.*event" "*test*"
# Search for comments or documentation mentioning travel history
rg -i "travel.*(history|audit|track)"
Length of output: 167
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Find all test files that might reference Travel events
fd "test.*\.(cairo|rs|ts|js)" | xargs rg -l "Travel"
# 2. Search for any code that processes or queries Travel events
rg -l "Travel" --type cairo --type rust --type typescript --type javascript
# 3. Look for documentation about travel mechanics
fd "README|CONTRIBUTING|ARCHITECTURE|docs" | xargs rg -i "travel"
# 4. Search for event processing or historical data handling
rg -i "(historical|history|audit).*event|event.*(historical|history|audit)"
Length of output: 3238
Script:
#!/bin/bash
# Let's examine the test files and related code more closely
# Check test files for historical event usage
rg -A 5 "Travel" contracts/src/systems/transport/tests/travel_systems_tests.cairo
rg -A 5 "Travel" contracts/src/systems/map/tests.cairo
# Check if any systems are querying historical travel events
ast-grep --pattern 'fn $_($$) {
$$$
Travel $$$
$$$
}'
# Look for event handling in client code
rg -l "Travel.*Event" client/src/
Length of output: 1941
contracts/src/systems/map/contracts.cairo (2)
Line range hint 1-300: Implementation looks solid and well-structured
The code demonstrates:
- Good separation of concerns
- Proper error handling
- Secure random number generation with multiple entropy sources
- Clear business logic implementation
54-54: Verify impact of non-historical events on existing queries
The change from historical to non-historical events for both MapExplored and FragmentMineDiscovered could affect existing queries and data retention. While this change can improve performance and reduce storage costs, it's important to ensure that:
- No critical historical data is lost
- Existing queries are updated to handle non-historical events
- Systems depending on historical event data are modified accordingly
Also applies to: 71-71
✅ Verification successful
Let me gather more specific information about how these events are being used in the frontend and GraphQL schema.
Based on the results, I can now generate the final response since I have enough information to verify the impact of making these events non-historical.
Non-historical events change is safe to proceed
The change from historical to non-historical events for MapExplored and FragmentMineDiscovered is safe because:
- The frontend only uses these events for real-time activity feed display (as seen in
eventDetails.tsxandtypes.tsx) - No critical historical data queries or permanent storage requirements were found
- The events are used for transient game state updates rather than long-term record keeping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for event queries that might be affected
# Look for imports or uses of these events
rg -l "MapExplored|FragmentMineDiscovered"
# Look for event query patterns that might need updates
ast-grep --pattern 'world.emit_event($$$)'
Length of output: 407
Script:
#!/bin/bash
# Check GraphQL schema for event queries
cat landing/schema.graphql
# Check frontend usage of these events
rg -A 5 "MapExplored|FragmentMineDiscovered" client/src/ui/modules/stream/types.tsx client/src/ui/modules/stream/eventDetails.tsx
# Check if there are any tests relying on historical events
rg -A 5 "MapExplored|FragmentMineDiscovered" contracts/src/utils/testing/world.cairo
Length of output: 225139
contracts/src/systems/resources/contracts/resource_systems.cairo (2)
Line range hint 47-54: Ensure consistent event historicity across modules
This change aligns with the broader architectural decision to make events non-historical across modules. However, we should verify that this change is consistent with other event modifications in the PR.
Let's verify the consistency of event historical attributes:
#!/bin/bash
# Search for any remaining historical events
ast-grep --pattern 'historical: true' --lang cairo
# List all event structs to ensure consistent treatment
ast-grep --pattern '#[dojo::event]
struct $EVENT_NAME {
$$$
}' --lang cairo47-47: Verify impact of disabling historical events
Making the Transfer event non-historical could impact features that rely on historical transfer data, such as:
- Auditing of resource transfers
- Analysis of transfer patterns
- Debugging of transfer-related issues
- Historical reporting capabilities
Let's verify if any code relies on historical transfer events:
✅ Verification successful
The previous commands failed because Cairo isn't a supported language for ast-grep and some syntax issues. Let's try with basic text search using ripgrep to find relevant code patterns.
Historical events change appears to be part of broader architectural update
Based on the search results, this change appears to be part of a broader architectural update across multiple systems. The Transfer event is used extensively throughout the codebase:
- Multiple contract systems reference Transfer events (resources, trade, transport, bank)
- Frontend components in client/src handle Transfer events for UI display
- SDK and GraphQL schema include Transfer-related functionality
Since this change is:
- Part of a broader architectural update to make events non-historical
- Still allows events to be captured by indexers and frontend components
- Maintains consistency with other system changes
The modification appears intentional and aligned with the codebase's evolution. Frontend components and indexers can still consume these events in real-time, they just won't be queryable through historical APIs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for code that might query historical Transfer events
rg -l "Transfer.*historical|historical.*Transfer" --type cairo
# Search for potential indexers or consumers of Transfer events
rg -l "Transfer.*event|event.*Transfer" --type cairo -g '!**/resource_systems.cairo'
Length of output: 474
Script:
#!/bin/bash
# Search for any references to Transfer events or historical events
rg -l "Transfer.*event|event.*Transfer|historical.*event|event.*historical"
# Search specifically for Transfer struct usage
rg -l "struct Transfer"
# Look for potential event consumers or indexers
rg -l "get_events|emit_event|Transfer"
Length of output: 2616
contracts/src/systems/trade/contracts/trade_systems.cairo (1)
65-65: Verify implications of non-historical events
The modification of all trade-related events to historical: false could impact data retention and system observability. This change appears to be part of a broader architectural shift, as indicated by the blocking PR in dojo.js (#329).
Consider documenting:
- The rationale behind removing historical events
- How historical data will be handled/accessed if needed
- Any performance implications or benefits
Also applies to: 76-76, 89-89, 100-100
✅ Verification successful
Let me gather more specific information about the event usage and dependencies.
Based on the gathered information, I can now provide the final response:
Setting historical: false for trade events is safe to proceed
The change to non-historical events in the trade system is safe as:
- No trade-related components rely on historical event querying
- The found history references (
PillageHistory.tsx,TheWorld.tsx,LocationManager.ts) are unrelated to trade events - All trade events are consistently modified to be non-historical
- Event emission patterns remain unchanged, only the historical flag is modified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in event historical settings across the codebase
# and verify if any components depend on historical events
# Check for any remaining historical events
echo "Checking for any remaining historical events..."
rg -l "historical:\s*true" .
# Check for potential dependencies on historical events
echo "Checking for potential dependencies on historical events..."
rg -l "get_events|get_historical|history" .
Length of output: 503
Script:
#!/bin/bash
# Check the content of files that might depend on historical events
echo "Checking PillageHistory.tsx..."
rg -A 5 "history" ./client/src/ui/components/military/PillageHistory.tsx
echo -e "\nChecking TheWorld.tsx..."
rg -A 5 "history" ./client/src/ui/components/hints/TheWorld.tsx
echo -e "\nChecking LocationManager.ts..."
rg -A 5 "history" ./client/src/three/helpers/LocationManager.ts
# Check if there are any other event-related patterns in the trade system
echo -e "\nChecking trade system for event patterns..."
rg -A 5 "event" ./contracts/src/systems/trade/
Length of output: 6529
contracts/src/systems/hyperstructure/contracts.cairo (3)
Line range hint 82-91: LGTM! Contribution tracking remains intact.
The change to historical: false is safe as all contribution data needed for points calculation is properly stored in the Progress and Contribution models.
Line range hint 60-99: Ensure dojo.js version compatibility before merging.
This change depends on dojo.js PR #329. Ensure that:
- The dojo.js PR is merged first
- The dojo.js version is bumped in this project
- All affected event queries in the frontend are updated
#!/bin/bash
# Description: Check dojo.js version and usage
# Check package.json for dojo.js version
fd -t f "package.json" -x cat {} \; | jq '.dependencies["@dojoengine/core"]'
# Search for event queries in frontend code
fd -e ts -e tsx -x rg -l "world\.events\(\)"Line range hint 60-66: Verify if any external systems depend on historical HyperstructureFinished events.
While changing to historical: false is safe for internal functionality since completion status is tracked in the Hyperstructure model, we should verify no external systems rely on querying historical finish events.
✅ Verification successful
Changing historical attribute to false is safe for HyperstructureFinished events
Based on the codebase analysis, the HyperstructureFinished events are only used for real-time leaderboard updates and points calculation in the client, specifically in:
LeaderboardManager.ts: Processes events to calculate points and rankingsuseLeaderBoardStore.tsx: Subscribes to events for UI updatesuseSubscriptionToHyperstructureEvents(): Handles real-time event processing
The completion status is permanently stored in the Hyperstructure model state, and no systems depend on querying historical events. The events are only used for immediate processing when emitted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any code that queries HyperstructureFinished events
# This helps identify potential dependencies on historical events
# Search for direct references to HyperstructureFinished
rg -l "HyperstructureFinished"
# Search for event query patterns that might reference this event
ast-grep --pattern 'world.events()' -l
Length of output: 470
Script:
#!/bin/bash
# Search for direct references to HyperstructureFinished in all files
rg -l "HyperstructureFinished"
# Search for event query patterns in TypeScript files
rg -t ts "world\.events\(\)" -A 3
# Search for any references to hyperstructure events or queries
rg -i "hyperstructure.*event" -A 2
# Search specifically in test files for event usage
fd -e test.ts -e test.tsx -x rg "HyperstructureFinished" '{}' ';'
# Look for any event subscriptions or listeners
rg "subscribe.*Hyperstructure|on.*Hyperstructure"
Length of output: 45841
contracts/src/systems/combat/contracts/battle_systems.cairo (2)
Line range hint 318-323: Updated import statements reflect event handling changes
The import statements have been updated to include the specific event data types after the removal of EternumEvent, aligning with the refactored event handling structure.
Line range hint 804-809: Consistent updates to import statements in battle_pillage_systems module
The import statements in the battle_pillage_systems module have been updated similarly to reflect the changes in event handling, ensuring consistency across modules.
| @@ -31,7 +31,7 @@ mod liquidity_systems { | |||
| use eternum::systems::bank::contracts::bank::bank_systems::{InternalBankSystemsImpl}; | |||
|
|
|||
| #[derive(Copy, Drop, Serde)] | |||
| #[dojo::event(historical: true)] | |||
| #[dojo::event(historical: false)] | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
Non-historical liquidity events are actively used in the UI
The codebase analysis shows that liquidity events are actively used in several UI components and features:
LiquidityResourceRowcomponent usesgetLatestLiquidityEventto track and display liquidity changesMarketManagerrelies on historical events to calculate:- Price differences and returns for liquidity providers
- Latest liquidity positions
- Total liquidity metrics
Making these events non-historical will break these features as they depend on querying past liquidity operations. Please maintain the historical flag as true or implement alternative mechanisms to track this data before making this change.
🔗 Analysis chain
Verify implications of non-historical events
Making LiquidityEvent non-historical will affect the ability to query past liquidity operations. Please ensure that:
- No features or UI components rely on historical liquidity events
- No analytics or reporting requirements need historical event data
- Audit requirements for tracking liquidity changes are satisfied through other means
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UI components or features that might rely on historical liquidity events
rg -l "LiquidityEvent" --type ts --type tsx
# Check for any analytics or reporting code using these events
rg -l "liquidity.*events?" --type ts --type tsx
Length of output: 176
Script:
#!/bin/bash
# Search for any references to liquidity events in the codebase
rg -i "liquidity.*event" -A 3 -B 3
# Search for any event consumers or event handling logic
rg -i "on.*liquidity|handle.*liquidity" -A 3 -B 3
# Look for any event-related imports or dependencies
rg -i "import.*event.*liquidity|from.*event.*liquidity" -A 3 -B 3
# Check if there are any other historical events in the codebase
ast-grep --pattern '#[dojo::event(historical: $_)]'
Length of output: 49601
Blocked by dojoengine/dojo.js#329 and bumping dojojs to the appropriate version
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores