Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

revert api split changes #62

Merged
merged 1 commit into from
Mar 12, 2024
Merged

revert api split changes #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 67 additions & 91 deletions contracts/core/Dispatcher.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,35 +68,13 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable, Ibc {
}

/**
* This function is called by a 'relayer' on behalf of a dApp. The dApp should implement IbcChannelHandler's
* onChanOpenInit. If the callback succeeds, the dApp should return the selected version and the emitted event
* will be relayed to the IBC/VIBC hub chain.
* This func is called by a 'relayer' on behalf of a dApp. The dApp should be implements IbcChannelHandler.
* The dApp should implement the onOpenIbcChannel method to handle one of the first two channel handshake methods,
* ie. ChanOpenInit or ChanOpenTry.
* If callback succeeds, the dApp should return the selected version, and an emitted event will be relayed to the
* IBC/VIBC hub chain.
*/
function channelOpenInit(
IbcChannelReceiver portAddress,
string calldata version,
ChannelOrder ordering,
bool feeEnabled,
string[] calldata connectionHops,
string calldata counterpartyPortId
) external {
if (bytes(counterpartyPortId).length == 0) {
revert IBCErrors.invalidCounterPartyPortId();
}

string memory selectedVersion = portAddress.onChanOpenInit(version);

emit ChannelOpenInit(
address(portAddress), selectedVersion, ordering, feeEnabled, connectionHops, counterpartyPortId
);
}

/**
* This function is called by a 'relayer' on behalf of a dApp. The dApp should implement IbcChannelHandler's
* onChanOpenTry. If the callback succeeds, the dApp should return the selected version and the emitted event
* will be relayed to the IBC/VIBC hub chain.
*/
function channelOpenTry(
function openIbcChannel(
IbcChannelReceiver portAddress,
CounterParty calldata local,
ChannelOrder ordering,
Expand All @@ -109,15 +87,18 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable, Ibc {
revert IBCErrors.invalidCounterPartyPortId();
}

consensusStateManager.verifyMembership(
proof,
channelProofKey(local.portId, local.channelId),
channelProofValue(ChannelState.TRY_PENDING, ordering, local.version, connectionHops, counterparty)
);
if (_isChannelOpenTry(counterparty)) {
consensusStateManager.verifyMembership(
proof,
channelProofKey(local.portId, local.channelId),
channelProofValue(ChannelState.TRY_PENDING, ordering, local.version, connectionHops, counterparty)
);
}

string memory selectedVersion = portAddress.onChanOpenTry(counterparty.version);
string memory selectedVersion =
portAddress.onOpenIbcChannel(local.version, ordering, feeEnabled, connectionHops, counterparty);

emit ChannelOpenTry(
emit OpenIbcChannel(
address(portAddress),
selectedVersion,
ordering,
Expand All @@ -129,56 +110,43 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable, Ibc {
}

/**
* This func is called by a 'relayer' after the IBC/VIBC hub chain has processed the ChannelOpenTry event.
* The dApp should implement the onChannelConnect method to handle the third channel handshake method: ChanOpenAck
* This func is called by a 'relayer' after the IBC/VIBC hub chain has processed the onOpenIbcChannel event.
* The dApp should implement the onConnectIbcChannel method to handle the last two channel handshake methods, ie.
* ChanOpenAck or ChanOpenConfirm.
*/
function channelOpenAck(
function connectIbcChannel(
IbcChannelReceiver portAddress,
CounterParty calldata local,
string[] calldata connectionHops,
ChannelOrder ordering,
bool feeEnabled,
bool isChanConfirm,
CounterParty calldata counterparty,
Ics23Proof calldata proof
) external {
consensusStateManager.verifyMembership(
proof,
channelProofKey(local.portId, local.channelId),
channelProofValue(ChannelState.ACK_PENDING, ordering, local.version, connectionHops, counterparty)
);

_connectChannel(portAddress, local, connectionHops, ordering, feeEnabled, counterparty);

portAddress.onChanOpenAck(local.channelId, counterparty.version);
_verifyConnectIbcChannelProof(local, connectionHops, ordering, isChanConfirm, counterparty, proof);

emit ChannelOpenAck(address(portAddress), local.channelId);
}
portAddress.onConnectIbcChannel(local.channelId, counterparty.channelId, counterparty.version);

/**
* This func is called by a 'relayer' after the IBC/VIBC hub chain has processed the ChannelOpenTry event.
* The dApp should implement the onChannelConnect method to handle the last channel handshake method:
* ChannelOpenConfirm
*/
function channelOpenConfirm(
IbcChannelReceiver portAddress,
CounterParty calldata local,
string[] calldata connectionHops,
ChannelOrder ordering,
bool feeEnabled,
CounterParty calldata counterparty,
Ics23Proof calldata proof
) external {
consensusStateManager.verifyMembership(
proof,
channelProofKey(local.portId, local.channelId),
channelProofValue(ChannelState.CONFIRM_PENDING, ordering, local.version, connectionHops, counterparty)
// Register port and channel mapping
// TODO: check duplicated channel registration?
// TODO: The call to `Channel` constructor MUST be move to `openIbcChannel` phase
// Then `connectIbcChannel` phase can use the `version` as part of `require` condition.
portChannelMap[address(portAddress)][local.channelId] = Channel(
counterparty.version, // TODO: this should be self version instead of counterparty version
ordering,
feeEnabled,
connectionHops,
counterparty.portId,
counterparty.channelId
);

_connectChannel(portAddress, local, connectionHops, ordering, feeEnabled, counterparty);

portAddress.onChanOpenConfirm(local.channelId, counterparty.version);
// initialize channel sequences
nextSequenceSend[address(portAddress)][local.channelId] = 1;
nextSequenceRecv[address(portAddress)][local.channelId] = 1;
nextSequenceAck[address(portAddress)][local.channelId] = 1;

emit ChannelOpenConfirm(address(portAddress), local.channelId);
emit ConnectIbcChannel(address(portAddress), local.channelId);
Comment on lines +117 to +149
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The connectIbcChannel function manages the connection phase of the IBC channel handshake. Key points include:

  1. Proof Verification and Callback Invocation (Lines 127-129): The _verifyConnectIbcChannelProof internal function is used for proof verification, which is a good practice. The callback to portAddress.onConnectIbcChannel is crucial for the dApp to handle the connection phase. Ensure that the callback's implementation in dApps is robust and handles all edge cases.

  2. Channel Registration (Lines 131-141): The registration of the channel in the portChannelMap is a critical step. The comment on line 136 indicates a potential issue with using the counterparty version instead of the self version. This needs to be addressed to ensure that the channel information is correctly registered.

  3. Event Emission (Line 149): Emitting the ConnectIbcChannel event provides transparency for the channel connection process. Ensure that the event parameters are accurately populated.

  4. TODO Comments (Lines 132-134): The TODO comments suggest areas for improvement, such as checking for duplicated channel registration and moving the call to the Channel constructor to the openIbcChannel phase. These improvements should be prioritized to enhance the contract's robustness and clarity.

Line 136 uses the counterparty version instead of the self version for channel registration. This could lead to incorrect channel information being stored. Consider revising this to use the correct version.

Would you like assistance in addressing the TODO comments mentioned in lines 132-134? Implementing these improvements could enhance the contract's functionality and maintainability.

}

/**
Expand Down Expand Up @@ -485,31 +453,25 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable, Ibc {
emit SendPacket(sender, channelId, packet, sequence, timeoutTimestamp);
}

function _connectChannel(
IbcChannelReceiver portAddress,
function _verifyConnectIbcChannelProof(
CounterParty calldata local,
string[] calldata connectionHops,
ChannelOrder ordering,
bool feeEnabled,
CounterParty calldata counterparty
bool isChanConfirm,
CounterParty calldata counterparty,
Ics23Proof calldata proof
) internal {
// Register port and channel mapping
// TODO: check duplicated channel registration?
// TODO: The call to `Channel` constructor MUST be move to `openIbcChannel` phase
// Then `connectIbcChannel` phase can use the `version` as part of `require` condition.
portChannelMap[address(portAddress)][local.channelId] = Channel(
counterparty.version, // TODO: this should be self version instead of counterparty version
ordering,
feeEnabled,
connectionHops,
counterparty.portId,
counterparty.channelId
consensusStateManager.verifyMembership(
proof,
channelProofKey(local.portId, local.channelId),
channelProofValue(
isChanConfirm ? ChannelState.CONFIRM_PENDING : ChannelState.ACK_PENDING,
ordering,
local.version,
connectionHops,
counterparty
)
);

// initialize channel sequences
nextSequenceSend[address(portAddress)][local.channelId] = 1;
nextSequenceRecv[address(portAddress)][local.channelId] = 1;
nextSequenceAck[address(portAddress)][local.channelId] = 1;
}

// _isPacketTimeout returns true if the given packet has timed out acoording to host chain's block height and
Expand Down Expand Up @@ -553,4 +515,18 @@ contract Dispatcher is IbcDispatcher, IbcEventsEmitter, Ownable, Ibc {
addr := mload(add(addrBytes, 20))
}
}

// For XXXX => vIBC direction, SC needs to verify the proof of membership of TRY_PENDING
// For vIBC initiated channel, SC doesn't need to verify any proof, and these should be all empty
function _isChannelOpenTry(CounterParty calldata counterparty) internal pure returns (bool open) {
if (counterparty.channelId == bytes32(0) && bytes(counterparty.version).length == 0) {
open = false;
// ChanOpenInit with unknow conterparty
} else if (counterparty.channelId != bytes32(0) && bytes(counterparty.version).length != 0) {
// this is the ChanOpenTry; counterparty must not be zero-value
open = true;
} else {
revert IBCErrors.invalidCounterParty();
}
}
}
65 changes: 28 additions & 37 deletions contracts/core/UniversalChannelHandler.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,17 @@ contract UniversalChannelHandler is IbcReceiverBase, IbcUniversalChannelMW {
dispatcher.closeIbcChannel(channelId);
}

// IBC callback functions
function onConnectIbcChannel(bytes32 channelId, bytes32, string calldata counterpartyVersion)
external
onlyIbcDispatcher
{
if (keccak256(abi.encodePacked(counterpartyVersion)) != keccak256(abi.encodePacked(VERSION))) {
revert UnsupportedVersion();
}
connectedChannels.push(channelId);
}

function onCloseIbcChannel(bytes32 channelId, string calldata, bytes32) external onlyIbcDispatcher {
// logic to determin if the channel should be closed
bool channelFound = false;
Expand Down Expand Up @@ -138,43 +149,23 @@ contract UniversalChannelHandler is IbcReceiverBase, IbcUniversalChannelMW {
mwStackAddrs[mwBitmap] = mwAddrs;
}

// IBC callback functions
function onChanOpenAck(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher {
_connectChannel(channelId, counterpartyVersion);
}

function onChanOpenConfirm(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher {
_connectChannel(channelId, counterpartyVersion);
}

function onChanOpenInit(string calldata version)
external
view
onlyIbcDispatcher
returns (string memory selectedVersion)
{
return _openChannel(version);
}

function onChanOpenTry(string calldata counterpartyVersion)
external
view
onlyIbcDispatcher
returns (string memory selectedVersion)
{
return _openChannel(counterpartyVersion);
}

function _connectChannel(bytes32 channelId, string calldata version) private {
if (keccak256(abi.encodePacked(version)) != keccak256(abi.encodePacked(VERSION))) {
revert UnsupportedVersion();
}
connectedChannels.push(channelId);
}

function _openChannel(string calldata version) private pure returns (string memory selectedVersion) {
if (keccak256(abi.encodePacked(version)) != keccak256(abi.encodePacked(VERSION))) {
revert UnsupportedVersion();
function onOpenIbcChannel(
string calldata version,
ChannelOrder,
bool,
string[] calldata,
CounterParty calldata counterparty
) external view onlyIbcDispatcher returns (string memory selectedVersion) {
if (counterparty.channelId == bytes32(0)) {
// ChanOpenInit
if (keccak256(abi.encodePacked(version)) != keccak256(abi.encodePacked(VERSION))) {
revert UnsupportedVersion();
}
} else {
// ChanOpenTry
if (keccak256(abi.encodePacked(counterparty.version)) != keccak256(abi.encodePacked(VERSION))) {
revert UnsupportedVersion();
}
}
return VERSION;
}
Expand Down
84 changes: 45 additions & 39 deletions contracts/examples/Mars.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,22 @@ contract Mars is IbcReceiverBase, IbcReceiver {
timeoutPackets.push(packet);
}

function onConnectIbcChannel(bytes32 channelId, bytes32, string calldata counterpartyVersion)
external
onlyIbcDispatcher
{
// ensure negotiated version is supported
bool foundVersion = false;
for (uint256 i = 0; i < supportedVersions.length; i++) {
if (keccak256(abi.encodePacked(counterpartyVersion)) == keccak256(abi.encodePacked(supportedVersions[i]))) {
foundVersion = true;
break;
}
}
if (!foundVersion) revert UnsupportedVersion();
connectedChannels.push(channelId);
}

function onCloseIbcChannel(bytes32 channelId, string calldata, bytes32) external onlyIbcDispatcher {
// logic to determin if the channel should be closed
bool channelFound = false;
Expand Down Expand Up @@ -65,49 +81,39 @@ contract Mars is IbcReceiverBase, IbcReceiver {
dispatcher.sendPacket(channelId, bytes(message), timeoutTimestamp);
}

function onChanOpenAck(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher {
_connectChannel(channelId, counterpartyVersion);
}

function onChanOpenConfirm(bytes32 channelId, string calldata counterpartyVersion) external onlyIbcDispatcher {
_connectChannel(channelId, counterpartyVersion);
}

function onChanOpenInit(string calldata version)
external
view
onlyIbcDispatcher
returns (string memory selectedVersion)
{
return _openChannel(version);
}

function onChanOpenTry(string calldata counterpartyVersion)
external
view
onlyIbcDispatcher
returns (string memory selectedVersion)
{
return _openChannel(counterpartyVersion);
}

function _connectChannel(bytes32 channelId, string calldata counterpartyVersion) private {
// ensure negotiated version is supported
function onOpenIbcChannel(
string calldata version,
ChannelOrder,
bool,
string[] calldata,
CounterParty calldata counterparty
) external view onlyIbcDispatcher returns (string memory selectedVersion) {
if (bytes(counterparty.portId).length <= 8) {
revert IBCErrors.invalidCounterPartyPortId();
}
/**
* Version selection is determined by if the callback is invoked on behalf of ChanOpenInit or ChanOpenTry.
* ChanOpenInit: self version should be provided whereas the counterparty version is empty.
* ChanOpenTry: counterparty version should be provided whereas the self version is empty.
* In both cases, the selected version should be in the supported versions list.
*/
bool foundVersion = false;
selectedVersion =
keccak256(abi.encodePacked(version)) == keccak256(abi.encodePacked("")) ? counterparty.version : version;
for (uint256 i = 0; i < supportedVersions.length; i++) {
if (keccak256(abi.encodePacked(counterpartyVersion)) == keccak256(abi.encodePacked(supportedVersions[i]))) {
connectedChannels.push(channelId);
return;
if (keccak256(abi.encodePacked(selectedVersion)) == keccak256(abi.encodePacked(supportedVersions[i]))) {
foundVersion = true;
break;
}
}
revert UnsupportedVersion();
}

function _openChannel(string calldata version) private view returns (string memory selectedVersion) {
for (uint256 i = 0; i < supportedVersions.length; i++) {
if (keccak256(abi.encodePacked(version)) == keccak256(abi.encodePacked(supportedVersions[i]))) {
return version;
if (!foundVersion) revert UnsupportedVersion();
// if counterpartyVersion is not empty, then it must be the same foundVersion
if (keccak256(abi.encodePacked(counterparty.version)) != keccak256(abi.encodePacked(""))) {
if (keccak256(abi.encodePacked(counterparty.version)) != keccak256(abi.encodePacked(selectedVersion))) {
revert VersionMismatch();
}
}
revert UnsupportedVersion();

return selectedVersion;
}
}
Loading
Loading