Updated contract ABIs after sub-finality #33
Conversation
WalkthroughThe pull request introduces significant modifications across multiple ABI files, including Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
src/abi/fee.ts (2)
13-13: Consider adding the return type foracceptOwnershipfunction.The
acceptOwnershipfunction is correctly added and follows the Ownable2Step pattern. However, for consistency with other functions likeowner, consider specifying the return type:- acceptOwnership: fun("0x79ba5097", {}, ), + acceptOwnership: fun("0x79ba5097", {}, p.address),This change would make the function definition consistent with others and provide clearer information about the function's behavior.
Line range hint
1-64: Summary: Ownership management enhancements implemented correctly.The changes in this file consistently implement the Ownable2Step pattern, enhancing the contract's functionality for ownership management. New events, functions, and types have been added following existing patterns in the file. These additions provide a more robust ownership transfer mechanism, allowing for a two-step process that improves security.
Key changes include:
- New
OwnershipTransferStartedevent- New
acceptOwnershipandpendingOwnerfunctions- Corresponding type definitions for new events and functions
These changes align well with best practices for upgradeable contracts and provide better safeguards against accidental ownership transfers.
One minor suggestion was made to add a return type to the
acceptOwnershipfunction for consistency.abi/fee.json (1)
235-260: LGTM: New error definitions added with a minor suggestionThe new error definitions
FeeThresholdNotMet,IncorrectFeeSent, andSenderNotDispatcherhave been correctly implemented. These custom errors provide more specific information about failure conditions, which is beneficial for debugging and user interaction.Consider adding a
thresholdparameter to theFeeThresholdNotMeterror to provide more context:"type": "error", "name": "FeeThresholdNotMet", - "inputs": [] + "inputs": [ + { + "name": "threshold", + "type": "uint256", + "internalType": "uint256" + } + ]This addition would make it easier for users to understand the minimum required fee.
src/handlers/channels.ts (1)
Line range hint
1-24: Suggestion: Enhance test coverage for channel handling logicWhile the typo fix is the only highlighted change, it's worth noting that this file contains complex logic for managing channel states and updating metrics. To ensure the reliability and correctness of these critical operations, I recommend:
- Implementing comprehensive unit tests covering various scenarios for each function.
- Adding integration tests to verify the interaction between different channel states.
- Considering the use of property-based testing to explore edge cases in the channel lifecycle.
Enhancing test coverage will help maintain the integrity of the channel handling logic as the codebase evolves.
abi/uch.json (2)
2-6: Constructor and receive function addedThe constructor is now explicitly defined with
nonpayablestateMutability, and areceivefunction has been added withpayablestateMutability. This allows the contract to receive Ether directly.Ensure that the contract has proper mechanisms to handle and manage received Ether, as it can now accept direct transfers.
751-846: New events addedSeveral new events have been added:
AdminChanged,BeaconUpgraded,Initialized,OwnershipTransferred,UCHPacketSent, andUpgraded. These events are consistent with standard upgradeable and ownable contract patterns, and provide better traceability for important contract state changes.Ensure that these events are emitted at appropriate points in the contract to maintain transparency and allow for off-chain tracking of important state changes.
src/abi/dispatcher.ts (2)
154-155: Redundant Parameter Type forFeeVaultFunctionThe
FeeVaultParamstype is defined butfeeVaultdoes not accept any parameters. Consider removingFeeVaultParamsif it's unnecessary.Apply this diff to remove the redundant type:
- export type FeeVaultParams = FunctionArguments<typeof functions.feeVault> export type FeeVaultReturn = FunctionReturn<typeof functions.feeVault>
169-171: Redundant Parameter Type forpendingOwnerFunctionThe
PendingOwnerParamstype is defined, butpendingOwnerdoes not accept any parameters. Consider removingPendingOwnerParamsto avoid confusion.Apply this diff to remove the redundant type:
- export type PendingOwnerParams = FunctionArguments<typeof functions.pendingOwner> export type PendingOwnerReturn = FunctionReturn<typeof functions.pendingOwner>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- abi/dispatcher.json (15 hunks)
- abi/fee.json (1 hunks)
- abi/uch.json (1 hunks)
- src/abi/dispatcher.ts (8 hunks)
- src/abi/fee.ts (3 hunks)
- src/handlers/channels.ts (1 hunks)
- src/utils/topics.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (41)
src/abi/fee.ts (5)
7-7: LGTM: New eventOwnershipTransferStartedadded correctly.The new event follows the existing pattern and matches the common Ownable contract implementation. The use of indexed parameters for
previousOwnerandnewOwneris correct and allows for efficient event filtering.
17-17: LGTM: New functionpendingOwneradded correctly.The
pendingOwnerfunction is properly defined in both thefunctionsobject and theContractclass. It follows the existing patterns and correctly specifies the return type asp.address. This addition complements the Ownable2Step pattern implementation.Also applies to: 29-31
36-36: LGTM: New event typeOwnershipTransferStartedEventArgsadded correctly.The new event type is properly defined and follows the existing pattern for event types in the file. It correctly uses the
EParamstype with theOwnershipTransferStartedevent.
41-42: LGTM: New function types foracceptOwnershipadded correctly.The new function types
AcceptOwnershipParamsandAcceptOwnershipReturnare properly defined and follow the existing pattern for function types in the file. They correctly use theFunctionArgumentsandFunctionReturntypes with theacceptOwnershipfunction.
53-54: LGTM: New function types forpendingOwneradded correctly.The new function types
PendingOwnerParamsandPendingOwnerReturnare properly defined and follow the existing pattern for function types in the file. They correctly use theFunctionArgumentsandFunctionReturntypes with thependingOwnerfunction.abi/fee.json (3)
2-8: LGTM: New ownership management functions addedThe new functions
acceptOwnership,pendingOwner,renounceOwnership,transferOwnership, andwithdrawFeesToOwnerhave been correctly implemented. These additions enhance the contract's ownership management capabilities and provide a secure way to transfer ownership.Also applies to: 83-95, 96-102, 103-115, 116-122
70-82: LGTM: owner function maintainedThe
ownerfunction signature remains unchanged, which maintains backwards compatibility.
123-234: LGTM: New events added for fee deposits and ownership transfersThe new events
OpenChannelFeeDeposited,OwnershipTransferStarted,OwnershipTransferred, andSendPacketFeeDepositedhave been correctly implemented. These events provide crucial information for tracking fee deposits and ownership changes, enhancing the contract's transparency and auditability.src/handlers/channels.ts (1)
22-22: Excellent fix: Corrected typo in event property nameThis change fixes a critical typo in the property name, changing
event.receviertoevent.receiver. This correction ensures that theportAddressis correctly derived from the event data, preventing potential bugs and improving the overall reliability of the function.To ensure consistency across the codebase, let's verify if similar corrections were made in other functions:
✅ Verification successful
Typo Correction Verified: 'recevier' not found
The search confirms that there are no remaining instances of the typo 'recevier' or similar misspellings in the TypeScript files. All occurrences of
receiverare correctly spelled, ensuring consistency and reliability across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of 'recevier' in the codebase rg 'recevier' --type tsLength of output: 23
Script:
#!/bin/bash # Verify that 'rg' is functioning correctly by searching for a common keyword, e.g., 'function' rg 'function' --type ts # Additionally, perform a case-insensitive search for 'recevier' to ensure no remaining typos rg -i 'recevier' --type ts # Search for similar typos that might have been introduced rg 'receiver'| 'reciever'| 'reciever' --type tsLength of output: 20850
Script:
#!/bin/bash # Verify that 'rg' is functioning correctly by searching for a common keyword, e.g., 'function' rg 'function' --type ts # Additionally, perform a case-insensitive search for 'recevier' to ensure no remaining typos rg -i 'recevier' --type ts # Search for similar typos that might have been introduced using a regex pattern rg -i 'recevier|reciever|receiver' --type tsLength of output: 23396
abi/uch.json (7)
11-23: Updated function signatures for MW_ID and VERSIONThe
MW_IDandVERSIONfunctions now have explicit input and output definitions, improving the ABI's clarity. Both are marked asviewfunctions, which is appropriate for read-only operations.Also applies to: 24-36
364-462: Updated packet handling and channel opening functionsThe
onRecvPacket,onTimeoutPacket, andopenChannelfunctions have been updated with explicit input and output definitions, improving type safety and ABI clarity.To ensure proper error handling in these functions, run:
#!/bin/bash # Check for error handling in packet and channel functions ast-grep --lang solidity --pattern 'function $_($$_) { $$$ require($_, $_); $$$ }'Also applies to: 463-543, 544-576
577-589: Updated ownership and upgradeable contract functionsThe
owner,proxiableUUID,registerMwStack, andrenounceOwnershipfunctions have been updated with explicit input and output definitions. These changes improve the contract's compatibility with standard ownership and upgradeable patterns.To ensure proper access control on ownership functions, run:
#!/bin/bash # Check for onlyOwner modifier usage ast-grep --lang solidity --pattern 'function $_($$_) $$$onlyOwner$$$ { $$$ }'Also applies to: 590-602, 603-620, 621-627
628-665: Updated packet sending, dispatcher setting, and upgrade functionsThe
sendMWPacket,sendUniversalPacket,setDispatcher,transferOwnership,upgradeTo, andupgradeToAndCallfunctions have been updated with explicit input and output definitions. TheupgradeToAndCallfunction is now marked aspayable, which is consistent with the OpenZeppelin upgradeable contracts pattern.To ensure proper implementation of upgrade functions, run:
#!/bin/bash # Check for upgrade function implementations ast-grep --lang solidity --pattern 'function upgrade$_($$_) { $$$ }'Also applies to: 666-693, 694-706, 707-719, 720-732, 733-750
847-866: New custom errors addedFour new custom errors have been added:
ChannelNotFound,MwBitmpaCannotBeZero,UnsupportedVersion, andnotIbcDispatcher. These custom errors can provide more gas-efficient and informative error handling compared to revert strings.There's a typo in the error name
MwBitmpaCannotBeZero. It should beMwBitmapCannotBeZero.To ensure proper usage of custom errors, run:
#!/bin/bash # Check for custom error usage ast-grep --lang solidity --pattern 'revert $_($$$);'
82-105: Updated IBC-related function signaturesThe IBC-related functions (
mwStackAddrs,onAcknowledgementPacket,onChanCloseConfirm,onChanCloseInit,onChanOpenAck,onChanOpenConfirm,onChanOpenInit,onChanOpenTry) have been updated with explicit input and output definitions, improving type safety and ABI clarity.To ensure all IBC-related functions are implemented correctly, run:
Also applies to: 106-203, 204-226, 227-249, 250-272, 273-285, 286-319, 320-363
37-55: Updated function signatures for state variables and initializationThe
connectedChannels,dispatcher, andinitializefunctions now have explicit input and output definitions. Theinitializefunction takes anIbcDispatchercontract address as input, which is consistent with upgradeable contract patterns.To ensure the
initializefunction is called only once, run the following script:Also applies to: 56-68, 69-81
abi/dispatcher.json (11)
7-13: Approval: NewacceptOwnershipfunction addedThe addition of the
acceptOwnershipfunction suggests the implementation of a two-step ownership transfer pattern. This is a security best practice that helps prevent accidental transfers to incorrect addresses.
151-232: Approval: New IBC channel closing functions addedThe addition of
channelCloseConfirmandchannelCloseInitfunctions enhances the contract's capabilities in managing IBC (Inter-Blockchain Communication) channels. These functions are crucial for properly closing channels in the IBC protocol, which is essential for maintaining the integrity of cross-chain communications.
channelCloseConfirm: Allows confirmation of a channel closure, likely initiated by the counterparty.channelCloseInit: Initiates the closure of a channel from this chain.These additions improve the contract's compliance with IBC specifications and provide more granular control over channel lifecycles.
Line range hint
1-2056: Summary: Comprehensive improvements to the Dispatcher contractThis review has covered significant changes to the Dispatcher contract's ABI, including:
- Addition of new functions for ownership management and IBC operations.
- Modifications to existing functions to enhance their functionality and return values.
- Introduction of new events for better observability of channel operations.
- Addition of specific error types for improved error handling and debugging.
Overall, these changes represent a substantial improvement to the contract's capabilities, particularly in areas of:
- IBC (Inter-Blockchain Communication) management
- Fee handling
- Ownership control
- Error reporting and handling
- System observability
The modifications appear well-thought-out and aligned with best practices in smart contract development. They should result in a more robust, flexible, and maintainable contract.
Key recommendations:
- Ensure thorough testing of all new and modified functions, especially focusing on edge cases and potential security implications.
- Update all related documentation, including any external systems or frontends that interact with this contract.
- Consider creating or updating integration tests that cover the new events and error scenarios.
- Perform a security audit, particularly focusing on the new connection management and ownership transfer functionalities.
1425-1500: Approval: New events added for channel closing operationsFour new events have been added to the contract:
ChannelCloseConfirmChannelCloseConfirmErrorChannelCloseInitChannelCloseInitErrorThese events correspond to the new channel closing functions and significantly improve the contract's observability. Their addition is a positive change that enhances the ability to monitor and react to channel lifecycle events.
Benefits:
- Improved tracking: External systems can now easily monitor the success or failure of channel closing operations.
- Better error handling: The separate error events allow for more granular error tracking and debugging.
- Enhanced auditability: These events provide a clear on-chain record of channel closing attempts and their outcomes.
- Easier integration: Other systems can use these events as triggers for further actions or notifications.
Recommendations:
- Ensure these events are emitted at the appropriate points in the channel closing functions.
- Update any off-chain monitoring systems to listen for and handle these new events.
- Consider adding additional indexed parameters to the events if filtering by specific fields (beyond the current
portAddressandchannelId) would be beneficial.To verify the correct implementation and emission of these events, run the following script:
#!/bin/bash # Search for event definitions rg --type solidity 'event\s+(ChannelCloseConfirm|ChannelCloseConfirmError|ChannelCloseInit|ChannelCloseInitError)' # Search for event emissions in the corresponding functions rg --type solidity 'emit\s+(ChannelCloseConfirm|ChannelCloseConfirmError|ChannelCloseInit|ChannelCloseInitError)'
Line range hint
962-981: Approval:sendPacketfunction now returns a sequence numberThe
sendPacketfunction has been modified to return auint64value representing the sequence number of the sent packet. This is a positive change that enhances the function's utility:
- It allows the caller to immediately know the sequence number of the packet they've just sent.
- This returned sequence can be used for better tracking and management of sent packets.
- It may simplify error handling and packet acknowledgment processes.
Benefits of this change:
- Improved packet tracking: Callers can easily associate their sent data with a specific sequence number.
- Enhanced error handling: If a send operation fails in a subsequent step, the caller knows exactly which sequence was affected.
- Simplified client-side operations: Clients no longer need to separately query for the sequence number of a just-sent packet.
Ensure that:
- All calls to
sendPacketare updated to handle the returned sequence number appropriately.- Any logic that previously had to query for a packet's sequence number after sending is refactored to use this returned value.
To verify the correct implementation and usage of the modified
sendPacketfunction, run the following script:#!/bin/bash # Search for the sendPacket function implementation rg --type solidity 'function\s+sendPacket.*returns\s*\(\s*uint64\s+\w+\s*\)' # Search for calls to sendPacket that now use the returned sequence rg --type solidity 'sendPacket\s*\(.*\)' -A 3
1974-2056: Approval: New error definitions added for improved error handlingSeveral new error definitions have been added to the contract, including:
channelIdNotFoundchannelNotOwnedByPortAddressinvalidConnectioninvalidConnectionHopsinvalidCounterPartyinvalidPortPrefixinvalidVersionlightClientNotFoundnotEnoughGasThese additions significantly enhance the contract's error handling capabilities, providing more specific and informative error messages. This is a positive change that will aid in debugging, maintenance, and overall contract robustness.
Benefits:
- Improved debugging: More specific error messages make it easier to identify and fix issues.
- Enhanced user experience: Callers of the contract can provide more meaningful error messages to end-users.
- Better contract integrity: Specific errors allow for more precise handling of edge cases and invalid inputs.
- Increased maintainability: Clear error definitions make the code more self-documenting and easier to maintain.
Recommendations:
- Ensure these new error types are used consistently throughout the contract where appropriate.
- Update any external systems or frontends that interact with this contract to handle and display these new error types.
- Consider adding documentation or comments explaining the specific scenarios where each error might be thrown.
To verify the correct implementation and usage of these new error types, run the following script:
#!/bin/bash # Search for error definitions rg --type solidity 'error\s+(channelIdNotFound|channelNotOwnedByPortAddress|invalidConnection|invalidConnectionHops|invalidCounterParty|invalidPortPrefix|invalidVersion|lightClientNotFound|notEnoughGas)' # Search for usage of these errors in revert statements rg --type solidity 'revert\s+(ChannelIdNotFound|ChannelNotOwnedByPortAddress|InvalidConnection|InvalidConnectionHops|InvalidCounterParty|InvalidPortPrefix|InvalidVersion|LightClientNotFound|NotEnoughGas)'
982-998: Approval: NewsetClientForConnectionfunction addedThe addition of the
setClientForConnectionfunction is a valuable enhancement to the contract's IBC capabilities. This function allows for more granular control over the association between connections and light clients.Key points:
- Takes two parameters:
connection(string) andlightClient(address of ILightClient contract).- It's a
nonpayablefunction, indicating it modifies the contract state.This function is crucial for IBC operations as it allows:
- Dynamic assignment or reassignment of light clients to specific connections.
- Potential for upgrading or changing light clients for a connection without affecting others.
- More flexible management of the IBC network topology.
Ensure that:
- Proper access controls are in place to prevent unauthorized modifications of connection-client associations.
- The function includes checks for valid connection strings and light client addresses.
- There's appropriate event emission to log these important state changes.
- The impact on existing packets or operations for the affected connection is considered and handled appropriately.
To verify the implementation and usage of this new function, please run the following script:
#!/bin/bash # Search for the setClientForConnection function implementation rg --type solidity 'function\s+setClientForConnection\s*\(\s*string\s+\w+\s*,\s*(?:address|ILightClient)\s+\w+\s*\)' # Search for access control modifiers on the setClientForConnection function rg --type solidity 'setClientForConnection.*\{' -A 5 # Search for event emissions related to setting client for connection rg --type solidity 'emit.*ClientSet.*Connection'
933-945: Approval: NewremoveConnectionfunction addedThe addition of the
removeConnectionfunction enhances the contract's ability to manage IBC connections dynamically. This is a significant feature that allows for the removal of connections, which can be crucial for maintaining the security and integrity of the IBC network.Key points:
- The function takes a single
connectionparameter of typestring.- It's a
nonpayablefunction, indicating that it modifies the contract state.While this addition provides more flexibility in managing connections, it's important to ensure:
- Proper access controls are in place to prevent unauthorized removal of connections.
- The function handles edge cases, such as attempting to remove a non-existent connection.
- There's a mechanism to handle any ongoing operations or pending packets for the connection being removed.
- The removal process properly cleans up all associated data to prevent orphaned or dangling references.
To verify the implementation and usage of this new function, please run the following script:
622-631:⚠️ Potential issueSignificant change:
closeIbcChannelfunction replaced withfeeVaultThe
closeIbcChannelfunction has been replaced with afeeVaultfunction, which represents a substantial change in functionality:
- The new function returns an address of type
IFeeVaultinstead of closing an IBC channel.- The state mutability has changed from
nonpayabletoview, indicating it no longer modifies the contract state.This change suggests a shift in the contract's fee management strategy and possibly a decoupling of fee-related functionality from IBC channel operations. Ensure that:
- The removal of
closeIbcChanneldoesn't break existing functionality or leave channels in an inconsistent state.- The new
feeVaultfunction is properly integrated with the rest of the contract's fee management system.- Any external contracts or systems that relied on
closeIbcChannelare updated accordingly.To verify the impact of this change, please run the following script:
Line range hint
724-730: Modified:initializefunction now usesIFeeVaultinstead oflightClientThe
initializefunction has been modified to use a fee vault instead of a light client:
- The second parameter has been renamed from
lightClientto_feeVault.- The type of this parameter has changed from
addresstocontract IFeeVault.This change is consistent with the earlier observed shift in fee management strategy. It suggests that the contract now initializes with a dedicated fee vault, potentially allowing for more sophisticated fee handling mechanisms.
Implications and action items:
- Ensure that all contract deployments and initializations are updated to provide an
IFeeVaultaddress instead of a light client address.- Verify that the
IFeeVaultcontract is properly implemented and deployed before initializing this contract.- Update any documentation or deployment scripts to reflect this change in the initialization process.
- Consider the impact on existing deployed contracts if this is an upgrade, as the initialization signature has changed.
To verify the correct implementation and usage of the new initialization process, run the following script:
696-706: 🛠️ Refactor suggestionModified:
getOptimisticConsensusStaterenamed and simplified togetStateThe function has been renamed from
getOptimisticConsensusStatetogetStateand its signature has been modified:
- An additional
connectionparameter of typestringhas been added.- The return type has been simplified from a struct to a single
uint256representing theappHash.These changes suggest a more flexible and possibly more efficient approach to retrieving state information. The addition of the
connectionparameter allows for state retrieval specific to a particular connection, which could be useful in a multi-chain environment.Ensure that:
- All calls to the old
getOptimisticConsensusStatefunction are updated to use the newgetStatefunction.- The simplified return value (appHash) is sufficient for all use cases that previously relied on the more complex struct return type.
- The new
connectionparameter is properly handled and validated within the function implementation.To verify the impact of this change and ensure all references are updated, run the following script:
src/abi/dispatcher.ts (14)
18-18: Typographical Correction AppliedGood job correcting the parameter name from
receviertoreceiverin theChannelOpenInitevent. This improves code clarity and prevents potential bugs.
68-70: Correct Implementation offeeVaultMethodThe
feeVaultmethod is correctly implemented to return the address of the fee vault. This allows for proper access to fee-related functionalities.
84-86: Verification ofpendingOwnerMethodThe
pendingOwnermethod retrieves the address of the pending owner. Ensure that this method is accurately reflecting the contract's state and that any ownership transfer logic is correctly handled.
106-109: Addition of Event Argument Types for New EventsThe types
ChannelCloseConfirmEventArgs,ChannelCloseConfirmErrorEventArgs,ChannelCloseInitEventArgs, andChannelCloseInitErrorEventArgshave been added. This enhances type safety and event handling consistency.
130-132: Addition of Types foracceptOwnershipFunctionThe
AcceptOwnershipParamsandAcceptOwnershipReturntypes have been added. This ensures type safety when callingacceptOwnership.
136-141: Addition of Types for Channel Close FunctionsThe types for
ChannelCloseConfirmandChannelCloseInitfunctions have been added, improving type safety and clarity.
160-161: Updated Types forgetStateFunctionThe types
GetStateParamsandGetStateReturnare updated. This ensures that parameter and return types match the function's updated signature.
184-186: Addition of Types forremoveConnectionFunctionThe
RemoveConnectionParamsandRemoveConnectionReturntypes have been added. This enhances type safety when using theremoveConnectionfunction.
193-195: Addition of Types forsetClientForConnectionFunctionThe types
SetClientForConnectionParamsandSetClientForConnectionReturnhave been added, improving type safety and function clarity.
205-206: Updated Types forupdateClientFunctionThe
UpdateClientParamsandUpdateClientReturntypes have been added, ensuring consistency with the updatedupdateClientfunction.
45-46: Verify Updated Function SignaturesThe
getStateandinitializefunctions have updated parameters. Ensure that these changes are consistent with the smart contract's ABI and that any calls to these functions in the codebase are updated accordingly.Run the following script to find and review usages:
#!/bin/bash # Description: Locate all calls to `getState` and `initialize` to verify parameter usage. rg --type ts --context 5 'getState|initialize'
37-38: Validate New Channel Close FunctionsThe functions
channelCloseConfirmandchannelCloseInithave been added. Ensure that their parameters and return types match the contract's ABI, and update any calling code to use these new functions appropriately.Run the following script to locate usages:
#!/bin/bash # Description: Find all instances where `channelCloseConfirm` and `channelCloseInit` are called. rg --type ts --context 5 'channelCloseConfirm|channelCloseInit'
35-35: Ensure Proper Integration ofacceptOwnershipFunctionThe
acceptOwnershipfunction has been added. Please verify that this function aligns with the ownership transfer logic in the contract and that all necessary access controls are in place.Run the following script to check usage and access control:
55-55: Update Return Value Handling forsendPacketFunctionThe
sendPacketfunction now returns ap.uint64. Ensure that any code handling the return value is updated to process thisuint64correctly.Run the following script to find usages and check return value handling:
| dispatcher.events.ChannelCloseInit.topic, | ||
| dispatcher.events.ChannelCloseConfirm.topic, |
There was a problem hiding this comment.
💡 Codebase verification
Found remaining references to CloseIbcChannel. Please update these to ChannelCloseInit and ChannelCloseConfirm:
src/server-extension/resolvers/stats.tssrc/model/generated/closeIbcChannel.model.tssrc/model/generated/channel.model.tssrc/handlers/index.ts
🔗 Analysis chain
LGTM! Verify impact on existing event listeners.
The addition of ChannelCloseInit and ChannelCloseConfirm topics is consistent with the existing structure and naming conventions. This change provides more granular control over channel closing events, which aligns with the PR objectives.
To ensure this change doesn't break existing functionality, please run the following script to check for any code still referencing the old CloseIbcChannel event:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the old CloseIbcChannel event
# Test: Search for CloseIbcChannel references
rg --type typescript --type javascript 'CloseIbcChannel'
# If any results are found, they may need to be updated to use the new ChannelCloseInit and ChannelCloseConfirm events.
Length of output: 91
Script:
#!/bin/bash
# Description: Search for any remaining references to the old CloseIbcChannel event in TypeScript and JavaScript files
# Test: Search for CloseIbcChannel references in .ts and .js files
rg 'CloseIbcChannel' --glob '*.ts' --glob '*.js'
# If any results are found, they may need to be updated to use the new ChannelCloseInit and ChannelCloseConfirm events.
Length of output: 777
| { | ||
| "type": "function", | ||
| "name": "depositOpenChannelFee", | ||
| "inputs": [ | ||
| { | ||
| "internalType": "address", | ||
| "name": "src", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "internalType": "string", | ||
| "name": "version", | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "internalType": "enum ChannelOrder", | ||
| "name": "ordering", | ||
| "type": "uint8" | ||
| }, | ||
| { | ||
| "internalType": "string[]", | ||
| "name": "connectionHops", | ||
| "type": "string[]" | ||
| }, | ||
| { | ||
| "internalType": "string", | ||
| "name": "counterpartyPortId", | ||
| "type": "string" | ||
| } | ||
| { | ||
| "name": "src", | ||
| "type": "address", | ||
| "internalType": "address" | ||
| }, | ||
| { | ||
| "name": "version", | ||
| "type": "string", | ||
| "internalType": "string" | ||
| }, | ||
| { | ||
| "name": "ordering", | ||
| "type": "uint8", | ||
| "internalType": "enum ChannelOrder" | ||
| }, | ||
| { | ||
| "name": "connectionHops", | ||
| "type": "string[]", | ||
| "internalType": "string[]" | ||
| }, | ||
| { | ||
| "name": "counterpartyPortId", | ||
| "type": "string", | ||
| "internalType": "string" | ||
| } | ||
| ], | ||
| "name": "depositOpenChannelFee", | ||
| "outputs": [], | ||
| "stateMutability": "payable", | ||
| "type": "function" | ||
| }, | ||
| { | ||
| "stateMutability": "payable" | ||
| }, |
There was a problem hiding this comment.
Inconsistency: depositOpenChannelFee and depositSendPacketFee state mutability
The depositOpenChannelFee and depositSendPacketFee functions have had their state mutability changed from "payable" to "nonpayable". This change seems inconsistent with their purpose of depositing fees, as they typically need to receive Ether.
Please verify if this change is intentional. If not, consider reverting the state mutability back to "payable":
- "stateMutability": "payable"
+ "stateMutability": "nonpayable"If this change is intentional, please update the function names to reflect their new behavior (e.g., registerOpenChannelFee and registerSendPacketFee) and ensure that the contract logic has been updated accordingly.
Also applies to: 42-69
| getOptimisticConsensusState: fun("0xeb86ffdb", {"height": p.uint256}, {"appHash": p.uint256, "fraudProofEndTime": p.uint256, "ended": p.bool}), | ||
| initialize: fun("0x7ab4339d", {"initPortPrefix": p.string, "lightClient": p.address}, ), | ||
| feeVault: fun("0x478222c2", {}, p.address), | ||
| getChannel: fun("0x42852d24", {"portAddress": p.address, "channelId": p.bytes32}, p.struct({"version": p.string, "ordering": p.uint8, "feeEnabled": p.bool, "connectionHops": p.array(p.string), "counterpartyPortId": p.string, "counterpartyChannelId": p.bytes32, "portId": p.string})), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent Parameter Naming in getChannel Function
The getChannel function uses portAddress as a parameter. In other parts of the codebase, portId might be used for similar purposes. For consistency and to avoid confusion, consider renaming portAddress to portId if it represents an identifier rather than an address.
Apply this diff to rename the parameter:
- getChannel: fun("0x42852d24", {"portAddress": p.address, "channelId": p.bytes32}, p.struct({...})),
+ getChannel: fun("0x42852d24", {"portId": p.string, "channelId": p.bytes32}, p.struct({...})),And update the corresponding method:
- getChannel(portAddress: GetChannelParams["portAddress"], channelId: GetChannelParams["channelId"]) {
- return this.eth_call(functions.getChannel, {portAddress, channelId})
+ getChannel(portId: GetChannelParams["portId"], channelId: GetChannelParams["channelId"]) {
+ return this.eth_call(functions.getChannel, {portId, channelId})
}Committable suggestion was skipped due to low confidence.
| ChannelCloseConfirm: event("0x5f010dbbd6bf46aec8131c5456301a75cd688d3cca9bc8380c9e26301be20729", {"portAddress": indexed(p.address), "channelId": indexed(p.bytes32)}), | ||
| ChannelCloseConfirmError: event("0xc9d36d7a317cb116925d5cb66f0069fe825822fe23e9cf3f421c38cf444caa30", {"receiver": indexed(p.address), "error": p.bytes}), | ||
| ChannelCloseInit: event("0x21372e37743553ba8e5f61239803174a827c7732d53ec8adcb76c6b3bb2c13f1", {"portAddress": indexed(p.address), "channelId": indexed(p.bytes32)}), | ||
| ChannelCloseInitError: event("0xb1be59c1bcd39c54c7132a8e0d321af5db427575ddb3265560d8862804f4381b", {"receiver": indexed(p.address), "error": p.bytes}), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent Parameter Naming in Event Definitions
The events ChannelCloseConfirm and ChannelCloseInit use portAddress as a parameter, while similar events like ChannelOpenInit and ChannelOpenAck use receiver. For consistency and to avoid confusion, consider standardizing the parameter names across all channel-related events. This will enhance readability and maintainability.
Apply this diff to rename portAddress to receiver:
- ChannelCloseConfirm: event("0x5f010dbbd6bf46aec8131c5456301a75cd688d3cca9bc8380c9e26301be20729", {"portAddress": indexed(p.address), "channelId": indexed(p.bytes32)}),
+ ChannelCloseConfirm: event("0x5f010dbbd6bf46aec8131c5456301a75cd688d3cca9bc8380c9e26301be20729", {"receiver": indexed(p.address), "channelId": indexed(p.bytes32)}),
- ChannelCloseInit: event("0x21372e37743553ba8e5f61239803174a827c7732d53ec8adcb76c6b3bb2c13f1", {"portAddress": indexed(p.address), "channelId": indexed(p.bytes32)}),
+ ChannelCloseInit: event("0x21372e37743553ba8e5f61239803174a827c7732d53ec8adcb76c6b3bb2c13f1", {"receiver": indexed(p.address), "channelId": indexed(p.bytes32)}),📝 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.
| ChannelCloseConfirm: event("0x5f010dbbd6bf46aec8131c5456301a75cd688d3cca9bc8380c9e26301be20729", {"portAddress": indexed(p.address), "channelId": indexed(p.bytes32)}), | |
| ChannelCloseConfirmError: event("0xc9d36d7a317cb116925d5cb66f0069fe825822fe23e9cf3f421c38cf444caa30", {"receiver": indexed(p.address), "error": p.bytes}), | |
| ChannelCloseInit: event("0x21372e37743553ba8e5f61239803174a827c7732d53ec8adcb76c6b3bb2c13f1", {"portAddress": indexed(p.address), "channelId": indexed(p.bytes32)}), | |
| ChannelCloseInitError: event("0xb1be59c1bcd39c54c7132a8e0d321af5db427575ddb3265560d8862804f4381b", {"receiver": indexed(p.address), "error": p.bytes}), | |
| ChannelCloseConfirm: event("0x5f010dbbd6bf46aec8131c5456301a75cd688d3cca9bc8380c9e26301be20729", {"receiver": indexed(p.address), "channelId": indexed(p.bytes32)}), | |
| ChannelCloseConfirmError: event("0xc9d36d7a317cb116925d5cb66f0069fe825822fe23e9cf3f421c38cf444caa30", {"receiver": indexed(p.address), "error": p.bytes}), | |
| ChannelCloseInit: event("0x21372e37743553ba8e5f61239803174a827c7732d53ec8adcb76c6b3bb2c13f1", {"receiver": indexed(p.address), "channelId": indexed(p.bytes32)}), | |
| ChannelCloseInitError: event("0xb1be59c1bcd39c54c7132a8e0d321af5db427575ddb3265560d8862804f4381b", {"receiver": indexed(p.address), "error": p.bytes}), |
Summary by CodeRabbit
New Features
acceptOwnership,channelCloseConfirm, andchannelCloseInit.ChannelCloseConfirm,OwnershipTransferStarted, andOpenChannelFeeDeposited.Bug Fixes
Refactor
Documentation