Initial call handler #87
Conversation
WalkthroughThe pull request introduces significant updates across multiple files, primarily focusing on enhancing smart contract functionalities related to market operations. Key changes include the addition of various smart contract functions in Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
TomaszGol
force-pushed
the
CallHandler
branch
from
45be89b to
560a73c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (9)
src/types/v6.ts (3)
1-1: Consider removing unused importsThe import statement includes several items that are not used in this file (Result, Option, Bytes, BitSequence). To improve code cleanliness and potentially reduce bundle size, consider removing these unused imports.
You can update the import statement as follows:
import {sts} from './support'
3-8: LGTM! Consider minor formatting improvementThe
OrderSideconstant is well-defined usingsts.closedEnum. It correctly represents the two possible order sides (Long and Short) as simple flags.For improved readability, consider removing the extra space after the arrow function:
export const OrderSide: sts.Type<OrderSide> = sts.closedEnum(() => ({ Long: sts.unit(), Short: sts.unit(), }))
12-18: LGTM! Consider minor consistency improvementThe
OrderSide_LongandOrderSide_Shortinterfaces are well-defined and provide clear type information for the two possible order sides. The use of the__kindproperty with literal types is a good practice for discriminated unions in TypeScript.For consistency with the rest of the file, consider adding a blank line between the two interface definitions:
export interface OrderSide_Long { __kind: 'Long' } export interface OrderSide_Short { __kind: 'Short' }src/abi/abi.json (1)
1-250: Overall, the ABI is well-structured with a comprehensive set of market operations.The ABI defines a robust set of functions for managing markets, orders, and positions. The consistent use of parameter types (uint64 for market and position IDs, uint256 for prices and amounts, etc.) across functions is commendable.
General considerations for implementation:
- Ensure proper access control for critical functions (e.g.,
create_market,delete_market,oracle_price).- Implement thorough input validation and error handling.
- Consider the gas implications of using string parameters (in
create_market) and large integer types.- Verify that the contract handles edge cases, such as market deletions with open positions or orders.
- Consider adding events for important state changes to facilitate off-chain tracking and UI updates.
src/utils/getCall.ts (1)
35-35: Provide more detailed error message when decoding failsInclude additional information in the error message to aid in debugging when decoding the EVM call fails.
Apply this diff to improve the error message:
- console.error("Failed to decode the EVM call."); + console.error(`Failed to decode the EVM call for function ${functionName}.`);src/abi/abi.ts (1)
102-115: Clarify type aliases for 'create_order' function overloadsThe type aliases
Create_orderParams_0andCreate_orderParams_1use numeric suffixes, which may not clearly indicate the differences between the overloads.For better readability and maintenance, consider renaming the type aliases to more descriptive names that reflect their specific use cases. For example:
CreateOrderParamsWithMarginandCreateOrderParamsWithoutMarginCreateOrderParamsFullandCreateOrderParamsSimplifiedThis approach helps developers quickly understand the context and purpose of each type alias.
src/types/market/calls.ts (3)
129-129: Typo in comment: "positon" should be "position"There's a typographical error in the comment on line 129. The word "positon" should be corrected to "position".
Apply this diff to fix the typo:
- * Mark to market positon with oracle price + * Mark to market position with oracle price
143-143: Grammatical error in comment: "an user" should be "a user"There's a grammatical error in the comment on line 143. "An user" should be corrected to "a user". Additionally, consider rephrasing for clarity.
Apply this diff to fix the grammatical error:
- * Liquidate position of an user that has not sufficient margin + * Liquidate the position of a user who does not have sufficient margin
172-172: Typo in comment: "reduntant" should be "redundant"There's a typographical error in the comment on line 172. The word "reduntant" should be corrected to "redundant".
Apply this diff to fix the typo:
- * Withdraw reduntant funds from given market + * Withdraw redundant funds from the given market
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
- package.json (3 hunks)
- src/abi/abi.json (1 hunks)
- src/abi/abi.ts (1 hunks)
- src/types/calls.ts (1 hunks)
- src/types/index.ts (1 hunks)
- src/types/market/calls.ts (1 hunks)
- src/types/v6.ts (1 hunks)
- src/utils/getCall.ts (1 hunks)
- typegen.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/types/calls.ts
🧰 Additional context used
🔇 Additional comments (20)
src/types/index.ts (1)
2-2: Verify the existence and contents of thev6module.The addition of the
v6module export is a good practice for versioning and organizing type definitions. However, please ensure that:
- The
v6.tsfile exists in the same directory.- The exports from
v6.tsare intended to be exposed through this index file.- There's appropriate documentation for the new
v6types, especially if they introduce breaking changes compared to previous versions.To verify the existence and contents of the
v6module, please run the following script:src/types/v6.ts (1)
10-10: LGTM! Type definition is correctThe
OrderSidetype is correctly defined as a union ofOrderSide_LongandOrderSide_Short. This aligns with the constant defined earlier and provides a TypeScript-friendly way to use theOrderSidetype in the rest of the codebase.typegen.json (1)
31-33: LGTM! Verify type generation output.The addition of the Market pallet with
"calls": trueis correct and aligns with the PR objective of implementing an initial call handler. This change will enable the generation of call types for the Market pallet.To ensure the change has the intended effect, please verify the type generation output after this change. Run the following script to check for newly generated Market call types:
If no results are found, it may indicate that the type generation process needs to be re-run or that additional configuration might be required.
package.json (3)
5-5: Approved: Broadened Node.js version compatibilityUpdating the minimum Node.js version to 20 is a good decision. It allows the project to run on the current LTS version and improves compatibility while still leveraging modern features.
Line range hint
1-39: Summary of package.json changesThe changes to
package.jsonare well-considered and beneficial:
- Broadened Node.js compatibility improves deployment flexibility.
- Addition of
@subsquid/evm-typegenexpands the project's blockchain capabilities.- Upgrade to TypeScript 5 brings new features and improvements.
These updates enhance the project's capabilities and keep it up-to-date with current best practices. Please ensure to run the suggested verification scripts to confirm proper integration of these changes.
37-37: Approved: Updated TypeScript to version 5.6.3Updating to TypeScript 5.6.3 is a significant improvement that brings new features and enhancements. However, as this is a major version update, it's crucial to ensure compatibility with the existing codebase.
To verify compatibility and identify any necessary adjustments, please run the following script:
After running these checks, review any errors or warnings and make necessary adjustments to ensure full compatibility with TypeScript 5.
src/abi/abi.json (11)
1-19: LGTM:cancel_orderfunction definition is appropriate.The
cancel_orderfunction is well-defined with appropriate parameter types (uint64) for market and order_id. The nonpayable state mutability is correct for a function that modifies the contract state without accepting Ether.
20-47: LGTM:close_positionfunction definition is well-structured.The
close_positionfunction is correctly defined with appropriate parameter types. The use of uint64 for market and position_id, uint256 for price, and uint32 for quantity provides a good balance between range and gas efficiency.
48-90: LGTM:create_marketfunction is well-defined, with a minor consideration.The
create_marketfunction is correctly structured with appropriate parameter types for creating a new market. The use of string for market name and various uint types for other parameters is suitable.Note: The use of uint8 for initial_margin, maintenance_margin, and contract_unit_decimals is sufficient for most cases but might be limiting if you need to represent values greater than 255. Consider if uint16 would be more appropriate for future-proofing.
91-123: LGTM: Firstcreate_orderfunction is well-defined, with a note on enum representation.The function parameters are appropriately typed for creating a new order. The use of uint64 for market, uint256 for price, uint32 for quantity, and uint8 for margin provides a good balance between range and gas efficiency.
Note: The
order_sideparameter is defined as an enumMarket.OrderSidebut represented as uint8 in the ABI. This is correct as Solidity enums are represented as uint8 in the ABI, but ensure that the enum values in the contract match the expected uint8 values (0, 1, etc.) to avoid any confusion.
124-151: LGTM: Secondcreate_orderfunction provides a useful overload.This version of
create_orderis well-defined and consistent with the first version. The omission of themarginparameter provides a useful overload, likely using a default margin value in the contract implementation.Ensure that the contract logic handles the absence of the margin parameter appropriately, possibly by using a default value.
152-164: LGTM:delete_marketfunction is correctly defined, but consider its implications.The
delete_marketfunction is properly defined with a uint64 parameter for the market ID, consistent with other functions.Note: Deleting a market is a significant operation. Ensure that the contract implementation includes appropriate checks and balances, such as:
- Verifying that the caller has the necessary permissions to delete a market.
- Handling any existing orders or positions in the market before deletion.
- Considering whether a "soft delete" (marking as inactive) might be more appropriate than a hard delete in some cases.
183-200: LGTM:mark_to_marketfunction is well-defined.The
mark_to_marketfunction is correctly structured with appropriate parameter types. The use of uint64 for both market and position_id is consistent with other functions and suitable for the purpose.Ensure that the contract implementation handles the mark-to-market process correctly, including updating position values and handling any resulting actions (e.g., liquidations if the position falls below maintenance margin).
201-218: LGTM:oracle_pricefunction is correctly defined, but consider access control.The
oracle_pricefunction is well-structured with appropriate parameter types. The use of uint64 for market and uint256 for price provides sufficient range for most use cases.Note: Updating oracle prices is a critical operation that can significantly impact the system. Ensure that the contract implementation includes robust access control mechanisms to restrict this function to authorized oracle(s) only.
219-236: LGTM:withdrawfunction is correctly defined.The
withdrawfunction is well-structured with appropriate parameter types. The use of uint64 for market and uint256 for amount provides sufficient range for most use cases.Ensure that the contract implementation includes necessary checks, such as:
- Verifying that the user has sufficient balance to withdraw.
- Handling any open positions or orders that might be affected by the withdrawal.
- Implementing appropriate access control to ensure only authorized users can withdraw funds.
237-249: LGTM:withdraw_allfunction is correctly defined, but consider its implications.The
withdraw_allfunction is properly defined with a uint64 parameter for the market ID, consistent with other functions.Note: Withdrawing all funds from a market is a significant operation. Ensure that the contract implementation includes appropriate checks and balances, such as:
- Verifying that the user has no open positions or orders in the market before allowing a full withdrawal.
- Implementing appropriate access control to ensure only authorized users can withdraw all funds.
- Considering whether partial withdrawals might be more appropriate in some cases to maintain market liquidity.
165-182:⚠️ Potential issueConsider changing
depositfunction topayable.The
depositfunction is well-defined with appropriate parameter types. However, it's currently marked asnonpayable, which is unusual for a deposit function in Ethereum smart contracts.Suggestion: Consider changing the state mutability to
payableif the function is intended to receive Ether. If it's designed to handle token deposits, ensure that the contract has appropriate methods to handle the token transfer.src/abi/abi.ts (1)
1-144: Well-structured and type-safe ABI definitionsThe ABI definitions and associated type aliases are comprehensively set up, providing a clear and type-safe interface for interacting with the smart contract functions.
src/types/market/calls.ts (2)
178-178: Verify ifamountinwithdrawshould be optionalThe
amountparameter in thewithdrawfunction is defined as optional (sts.option(() => sts.bigint())), whereas in thedepositfunction,amountis required (sts.bigint()). Please confirm if it is intentional foramountto be optional inwithdraw. If not, consider making it consistent withdeposit.
205-205:⚠️ Potential issueUndefined
v7used incloseMarketexportThe
v7variable is used in thecloseMarketexport but is neither imported nor defined. This will cause aReferenceErrorat runtime. Please ensure thatv7is correctly imported or that the appropriate version variable is used.Apply this diff to fix the issue:
+import * as v7 from '../v7' ... - v7: new CallType( + v7: new CallType(Alternatively, if
v7is a typo and should refer to an existing version likev2orv6, update it accordingly.Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/eventprocessor/balances/reserveRepatriatedEventProcessor.ts (1)
35-52: Consider implementing structured logging and error monitoring.The new Ethereum transaction handling introduces additional complexity. To improve maintainability and debugging:
- Consider using a structured logging library (e.g., Winston, Pino) instead of console.log/error
- Add transaction tracing with correlation IDs
- Implement metrics collection for success/failure rates
Example structured logging implementation:
interface LogContext { eventId: string; callName?: string; marketId?: string; error?: Error; } function logError(message: string, context: LogContext) { // Integration with your logging infrastructure console.error({ message, timestamp: new Date().toISOString(), ...context, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- package.json (2 hunks)
- src/eventprocessor/balances/reserveRepatriatedEventProcessor.ts (2 hunks)
- src/eventprocessor/market/marketCreatedEventProcessor.ts (2 hunks)
- src/utils/getCall.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/utils/getCall.ts
🔇 Additional comments (5)
src/eventprocessor/market/marketCreatedEventProcessor.ts (2)
Line range hint
27-49: Consider handling call data for market creation.The method processes market creation events but doesn't utilize the newly added
callparameter. If the call contains relevant data for market creation (e.g., transaction details, caller information), it should be processed and stored.Let's check how other processors handle the call parameter:
#!/bin/bash # Description: Check how other processors utilize the call parameter # Search for patterns of call parameter usage rg -A 10 "async process.*call: Call.*?\{"
22-25: Verify the usage of the newcallparameter.The
processmethod has been updated to accept acallparameter, but it's not being utilized in the method implementation. This could indicate:
- Missing implementation of call processing logic
- Potential inconsistency with the event processor interface
Let's verify the interface implementation and usage across other event processors:
✅ Verification successful
Let me gather more information about the actual implementation of the MarketCreatedEventProcessor to see if it uses the
callparameter.
Let me try one more verification to check the interface and other implementations with a corrected path:
The
callparameter is correctly implemented according to the interfaceThe
callparameter is optional in theEventProcessorinterface (call?: Call<any>), and while it's not actively used in theMarketCreatedEventProcessor, this is acceptable. Some processors likeReserveRepatriatedEventProcessordo utilize thecallparameter for specific logic, showing that its presence in the interface and implementations is intentional and properly handled where needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check EventProcessor interface definition and its implementations # to verify if the `call` parameter is consistently handled # Check the EventProcessor interface definition echo "Checking EventProcessor interface definition:" ast-grep --pattern 'interface EventProcessor { $$$ process($$$): $$$ $$$ }' # Check other implementations of process method echo "Checking other event processor implementations:" rg -A 5 "implements EventProcessor.*?\{"Length of output: 9901
Script:
#!/bin/bash # Check the implementation of MarketCreatedEventProcessor's process method rg -A 20 "async process\(" "src/eventprocessor/market/marketCreatedEventProcessor.ts" # Check if any other processors are using the call parameter rg -A 5 "call\?" "src/eventprocessor/**/*.ts"Length of output: 974
Script:
#!/bin/bash # Check if any other processors are using the call parameter rg -A 5 "call\?" src/eventprocessor/ # Check for actual usage of call parameter in process methods rg -B 2 -A 10 "call: Call" src/eventprocessor/Length of output: 3935
src/eventprocessor/balances/reserveRepatriatedEventProcessor.ts (3)
20-20: LGTM: Import statement is correctly added.The new import is necessary for the Ethereum transaction processing functionality.
52-52:⚠️ Potential issueVerify market ID extraction and add validation.
The market ID extraction has changed from event arguments to call arguments, which could be a breaking change.
Let's verify the market ID format and usage:
#!/bin/bash # Search for market ID usage patterns rg "args\.market" # Search for Market type definitions ast-grep --pattern 'interface Market { $$$ }'Consider adding validation:
- const marketId = processedCall.args.market; + const marketId = processedCall.args.market; + if (!marketId || typeof marketId !== 'string') { + console.error(`Invalid market ID in processed call: ${JSON.stringify(processedCall.args)}`); + return; + }
35-48:⚠️ Potential issueAdd error handling and validation for Ethereum transaction processing.
The current implementation has several potential issues:
- Missing error handling for
processCall- No validation for event names
- Implicit type handling with undefined values
Consider applying these improvements:
- let ethCall; + let ethCall: ReturnType<typeof processCall> | undefined; if (call.name === "Ethereum.transact") { let callName: string | undefined; if (event.name === "Market.PositionMarkedToMarket") { callName = "mark_to_market"; } else if (event.name === "Market.PositionClosed") { callName = "close_position"; } - ethCall = processCall(call, callName); + try { + if (!callName) { + console.warn(`Unsupported event type: ${event.name}`); + return; + } + ethCall = processCall(call, callName); + } catch (error) { + console.error(`Failed to process Ethereum call: ${error}`); + throw error; + } } - const processedCall = ethCall ? ethCall : call; + const processedCall = ethCall ?? call;Let's verify the event names are consistent with the contract:
src/eventprocessor/balances/reserveRepatriatedEventProcessor.ts
Outdated
Show resolved
Hide resolved
src/eventprocessor/balances/reserveRepatriatedEventProcessor.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/eventprocessor/balances/reserveRepatriatedEventProcessor.ts (2 hunks)
- src/eventprocessor/market/marketCreatedEventProcessor.ts (2 hunks)
- src/utils/getCall.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/eventprocessor/balances/reserveRepatriatedEventProcessor.ts
- src/eventprocessor/market/marketCreatedEventProcessor.ts
🔇 Additional comments (2)
src/utils/getCall.ts (2)
1-1: LGTM: Clean imports sectionThe import is properly used throughout the code.
6-8: Verify ABI function selectorsLet's ensure all function selectors in the ABI are properly formatted.
✅ Verification successful
Let me gather more information about the actual selectors in the ABI file to better understand their format.
Let me try a different approach to locate and examine the ABI definitions.
Let's examine the content of the ABI files to understand the actual structure.
ABI function selectors are properly formatted
All function selectors in
src/abi/abi.tsfollow the correct format of "0x" followed by 8 hexadecimal characters (e.g., "0x9c5f444f", "0xf051c5da", etc.). The selectors are properly defined and match the function signatures in the ABI JSON. The code ingetCall.tsthat uses these selectors is correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all function selectors in the ABI are 10 characters long (0x + 8 chars) # Test: Search for selector definitions in abi.ts rg -A 1 'selector:' src/abi/abi.ts | grep -E '"0x[a-fA-F0-9]{8}"' || echo "Invalid selectors found"Length of output: 122
Script:
#!/bin/bash # Let's see the actual content of selector definitions rg -A 1 'selector:' src/abi/abi.tsLength of output: 34
Script:
#!/bin/bash # First, let's find all potential ABI-related files fd abi # Then let's look for any files that might contain function selectors rg -l "selector"Length of output: 87
Script:
#!/bin/bash # Check the content of abi.json cat src/abi/abi.json # Check the content of abi.ts cat src/abi/abi.tsLength of output: 9083
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores