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

DRAFT: Convert error to Acknowledgements #115

Closed
wants to merge 3 commits into from
Closed

DRAFT: Convert error to Acknowledgements #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
081be70
poc
AdityaSripal Nov 20, 2024
70c5ace
fix tests except fixtures
AdityaSripal Nov 20, 2024
abce6c7
use require rather than error acks
AdityaSripal Dec 4, 2024
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
15 changes: 4 additions & 11 deletions src/ICS20Transfer.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,24 +113,17 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
/// @inheritdoc IIBCApp
function onRecvPacket(OnRecvPacketCallback calldata msg_) external onlyOwner nonReentrant returns (bytes memory) {
// Since this function mostly returns acks, also when it fails, the ics26router (the caller) will log the ack
if (keccak256(bytes(msg_.payload.version)) != keccak256(bytes(ICS20Lib.ICS20_VERSION))) {
// TODO: Figure out if should actually error out, or if just error acking is enough
return ICS20Lib.errorAck(abi.encodePacked("unexpected version: ", msg_.payload.version));
}
require(keccak256(bytes(msg_.payload.version)) == keccak256(bytes(ICS20Lib.ICS20_VERSION)), IICS20Errors.ICS20UnexpectedVersion(ICS20Lib.ICS20_VERSION, msg_.payload.version));

ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(msg_.payload.value);
(address erc20Address, bool originatorChainIsSource) = getReceiveERC20AddressAndSource(
msg_.payload.sourcePort, msg_.sourceChannel, msg_.payload.destPort, msg_.destinationChannel, packetData
);

if (packetData.amount == 0) {
return ICS20Lib.errorAck("invalid amount: 0");
}
require(packetData.amount != 0, IICS20Errors.ICS20InvalidAmount(packetData.amount));

(address receiver, bool receiverConvertSuccess) = ICS20Lib.hexStringToAddress(packetData.receiver);
if (!receiverConvertSuccess) {
return ICS20Lib.errorAck(abi.encodePacked("invalid receiver: ", packetData.receiver));
}
require(receiverConvertSuccess, IICS20Errors.ICS20InvalidAddress(packetData.receiver));

if (originatorChainIsSource) {
// sender is source, so we mint vouchers
Expand All @@ -151,7 +144,7 @@ contract ICS20Transfer is IIBCApp, IICS20Transfer, IICS20Errors, Ownable, Reentr
function onAcknowledgementPacket(OnAcknowledgementPacketCallback calldata msg_) external onlyOwner nonReentrant {
ICS20Lib.PacketDataJSON memory packetData = ICS20Lib.unmarshalJSON(msg_.payload.value);

if (keccak256(msg_.acknowledgement) != ICS20Lib.KECCAK256_SUCCESSFUL_ACKNOWLEDGEMENT_JSON) {
if (!msg_.recvSuccess) {
(address erc20Address,) =
getSendERC20AddressAndSource(msg_.payload.sourcePort, msg_.sourceChannel, packetData);
_refundTokens(packetData, erc20Address);
Expand Down
36 changes: 24 additions & 12 deletions src/ICS26Router.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,18 +147,25 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
}

bytes[] memory acks = new bytes[](1);
acks[0] = getIBCApp(payload.destPort).onRecvPacket(
bool recvSuccess = true;
try getIBCApp(payload.destPort).onRecvPacket(
IIBCAppCallbacks.OnRecvPacketCallback({
sourceChannel: msg_.packet.sourceChannel,
destinationChannel: msg_.packet.destChannel,
sequence: msg_.packet.sequence,
payload: payload,
relayer: _msgSender()
})
);
require(acks[0].length != 0, IBCAsyncAcknowledgementNotSupported());
) returns (bytes memory ack) {
require(ack.length != 0, IBCAsyncAcknowledgementNotSupported());

acks[0] = ack;
} catch (bytes memory errorData) {
recvSuccess = false;
acks[0] = errorData;
}

writeAcknowledgement(msg_.packet, acks);
writeAcknowledgement(msg_.packet, recvSuccess, acks);

emit RecvPacket(msg_.packet);
}
Expand All @@ -181,7 +188,7 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
ICS24Host.packetAcknowledgementCommitmentPathCalldata(msg_.packet.destChannel, msg_.packet.sequence);
bytes[] memory acks = new bytes[](1);
acks[0] = msg_.acknowledgement;
bytes32 commitmentBz = ICS24Host.packetAcknowledgementCommitmentBytes32(acks);
bytes32 commitmentBz = ICS24Host.packetAcknowledgementCommitmentBytes32(msg_.recvSuccess, acks);

// verify the packet acknowledgement
ILightClientMsgs.MsgMembership memory membershipMsg = ILightClientMsgs.MsgMembership({
Expand All @@ -203,16 +210,19 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
return noopOnCorrectReason(reason, IICS24HostErrors.IBCPacketCommitmentNotFound.selector);
}

getIBCApp(payload.sourcePort).onAcknowledgementPacket(
try getIBCApp(payload.sourcePort).onAcknowledgementPacket(
IIBCAppCallbacks.OnAcknowledgementPacketCallback({
sourceChannel: msg_.packet.sourceChannel,
destinationChannel: msg_.packet.destChannel,
sequence: msg_.packet.sequence,
payload: payload,
recvSuccess: msg_.recvSuccess,
acknowledgement: msg_.acknowledgement,
relayer: _msgSender()
})
);
) {} catch {
// no-op
}
Comment on lines +223 to +225
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't actually be acking a packet if the callback cannot be made right? This feels wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

If the callback fails permanently then there's no point in resubmitting. This is for the case where ack just errors in an unrecoverable way. Resubmission won't fix anything here, so we may as well do cleanup.


emit AckPacket(msg_.packet, msg_.acknowledgement);
}
Expand Down Expand Up @@ -256,25 +266,27 @@ contract ICS26Router is IICS26Router, IICS26RouterErrors, Ownable, ReentrancyGua
return noopOnCorrectReason(reason, IICS24HostErrors.IBCPacketCommitmentNotFound.selector);
}

getIBCApp(payload.sourcePort).onTimeoutPacket(
try getIBCApp(payload.sourcePort).onTimeoutPacket(
IIBCAppCallbacks.OnTimeoutPacketCallback({
sourceChannel: msg_.packet.sourceChannel,
destinationChannel: msg_.packet.destChannel,
sequence: msg_.packet.sequence,
payload: payload,
relayer: _msgSender()
})
);
) {} catch {
// no-op
}
Comment on lines +277 to +279
Copy link
Member

Choose a reason for hiding this comment

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

Again, we need the timeout callback to succeed to unescrow funds

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto: What if the timeout callback can never succeed?

Copy link
Member

Choose a reason for hiding this comment

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

Then I think the packet should just be stuck?


emit TimeoutPacket(msg_.packet);
}

/// @notice Writes a packet acknowledgement and emits an event
/// @param packet The packet to acknowledge
/// @param acks The acknowledgement
function writeAcknowledgement(Packet calldata packet, bytes[] memory acks) private {
IBC_STORE.commitPacketAcknowledgement(packet, acks);
emit WriteAcknowledgement(packet, acks);
function writeAcknowledgement(Packet calldata packet, bool recvSuccess, bytes[] memory acks) private {
IBC_STORE.commitPacketAcknowledgement(packet, recvSuccess, acks);
emit WriteAcknowledgement(packet, recvSuccess, acks);
}

/// @notice No-op if the reason is correct, otherwise reverts with the same reason
Expand Down
3 changes: 2 additions & 1 deletion src/interfaces/IIBCStore.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ interface IIBCStore {

/// @notice Commits a packet acknowledgement
/// @param packet The packet to commit the acknowledgement for
/// @param recvSuccess The success of the receivePacket app callback
/// @param acks The list of acknowledgements (one for each payload) to commit
function commitPacketAcknowledgement(IICS26RouterMsgs.Packet memory packet, bytes[] memory acks) external;
function commitPacketAcknowledgement(IICS26RouterMsgs.Packet memory packet, bool recvSuccess, bytes[] memory acks) external;

// --------------------- Events --------------------- //

Expand Down
3 changes: 2 additions & 1 deletion src/interfaces/IICS26Router.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ interface IICS26Router is IICS26RouterMsgs {
event RecvPacket(Packet packet);
/// @notice Emitted when a packet acknowledgement is written
/// @param packet The packet that was acknowledged
/// @param recvSuccess The success of the receivePacket app callback
/// @param acknowledgements The list of acknowledgements data
event WriteAcknowledgement(Packet packet, bytes[] acknowledgements);
event WriteAcknowledgement(Packet packet, bool recvSuccess, bytes[] acknowledgements);
/// @notice Emitted when a packet is timed out
/// @param packet The packet that was timed out
event TimeoutPacket(Packet packet);
Expand Down
2 changes: 2 additions & 0 deletions src/msgs/IIBCAppCallbacks.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,15 @@ interface IIBCAppCallbacks {
/// @param destinationChannel The destination channel identifier
/// @param sequence The sequence number of the packet
/// @param payload The packet payload
/// @param recvSuccess the success boolean flag of the receive packet on counterparty
/// @param acknowledgement The acknowledgement
/// @param relayer The relayer of this message
struct OnAcknowledgementPacketCallback {
string sourceChannel;
string destinationChannel;
uint64 sequence;
IICS26RouterMsgs.Payload payload;
bool recvSuccess;
bytes acknowledgement;
address relayer;
}
Expand Down
2 changes: 2 additions & 0 deletions src/msgs/IICS26RouterMsgs.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,13 @@ interface IICS26RouterMsgs {

/// @notice Message for acknowledging packets, submitted by relayer
/// @param packet The packet to be acknowledged
/// @param recvSuccess The success of receivePacket app callback
/// @param acknowledgement The acknowledgement
/// @param proofAcked The proof of the acknowledgement commitment
/// @param proofHeight The proof height
struct MsgAckPacket {
Packet packet;
bool recvSuccess;
bytes acknowledgement;
bytes proofAcked;
IICS02ClientMsgs.Height proofHeight;
Expand Down
4 changes: 2 additions & 2 deletions src/utils/IBCStore.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ contract IBCStore is IIBCStore, IICS24HostErrors, Ownable {
}

/// @inheritdoc IIBCStore
function commitPacketAcknowledgement(IICS26RouterMsgs.Packet memory packet, bytes[] memory acks) public onlyOwner {
function commitPacketAcknowledgement(IICS26RouterMsgs.Packet memory packet, bool recvSuccess, bytes[] memory acks) public onlyOwner {
bytes32 path = ICS24Host.packetAcknowledgementCommitmentKeyCalldata(packet.destChannel, packet.sequence);
require(
commitments[path] == 0,
Expand All @@ -83,7 +83,7 @@ contract IBCStore is IIBCStore, IICS24HostErrors, Ownable {
)
);

bytes32 commitment = ICS24Host.packetAcknowledgementCommitmentBytes32(acks);
bytes32 commitment = ICS24Host.packetAcknowledgementCommitmentBytes32(recvSuccess, acks);
emit AckCommitted(path, commitment);
commitments[path] = commitment;
}
Expand Down
4 changes: 2 additions & 2 deletions src/utils/ICS24Host.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,13 @@ library ICS24Host {
/// @dev each payload get one ack each from their application, so this function accepts a list of acks
/// @param acks The list of acknowledgements to get the commitment for
/// @return The commitment bytes
function packetAcknowledgementCommitmentBytes32(bytes[] memory acks) internal pure returns (bytes32) {
function packetAcknowledgementCommitmentBytes32(bool recvSuccess, bytes[] memory acks) internal pure returns (bytes32) {
bytes memory ackBytes = "";
for (uint256 i = 0; i < acks.length; i++) {
ackBytes = abi.encodePacked(ackBytes, sha256(acks[i]));
}

return sha256(abi.encodePacked(uint8(2), ackBytes));
return sha256(abi.encodePacked(uint8(2), recvSuccess, ackBytes));
}

/// @notice Create a prefixed path
Expand Down
4 changes: 2 additions & 2 deletions test/BenchmarkTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ contract BenchmarkTest is FixtureTest {
recvFixture.packet.destChannel, recvFixture.packet.sequence
)
);
assertEq(storedAck, ICS24Host.packetAcknowledgementCommitmentBytes32(singleSuccessAck));
assertEq(storedAck, ICS24Host.packetAcknowledgementCommitmentBytes32(true, singleSuccessAck));
}

function test_ICS20TransferNativeSdkCoinWithSP1Fixtures_Plonk() public {
Expand All @@ -84,7 +84,7 @@ contract BenchmarkTest is FixtureTest {
recvNativeFixture.packet.destChannel, recvNativeFixture.packet.sequence
)
);
assertEq(storedAck, ICS24Host.packetAcknowledgementCommitmentBytes32(singleSuccessAck));
assertEq(storedAck, ICS24Host.packetAcknowledgementCommitmentBytes32(true, singleSuccessAck));
}

function test_ICS20TimeoutWithSP1Fixtures_Plonk() public {
Expand Down
13 changes: 10 additions & 3 deletions test/ICS20TransferTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ contract ICS20TransferTest is Test {
destinationChannel: packet.destChannel,
sequence: packet.sequence,
payload: packet.payloads[0],
recvSuccess: true,
acknowledgement: ICS20Lib.SUCCESSFUL_ACKNOWLEDGEMENT_JSON,
relayer: makeAddr("relayer")
})
Expand Down Expand Up @@ -419,6 +420,7 @@ contract ICS20TransferTest is Test {
destinationChannel: packet.destChannel,
sequence: packet.sequence,
payload: packet.payloads[0],
recvSuccess: false,
acknowledgement: ICS20Lib.FAILED_ACKNOWLEDGEMENT_JSON,
relayer: makeAddr("relayer")
})
Expand All @@ -444,6 +446,7 @@ contract ICS20TransferTest is Test {
destinationChannel: packet.destChannel,
sequence: packet.sequence,
payload: packet.payloads[0],
recvSuccess: false,
acknowledgement: ICS20Lib.FAILED_ACKNOWLEDGEMENT_JSON,
relayer: makeAddr("relayer")
})
Expand All @@ -459,6 +462,7 @@ contract ICS20TransferTest is Test {
destinationChannel: packet.destChannel,
sequence: packet.sequence,
payload: packet.payloads[0],
recvSuccess: false,
acknowledgement: ICS20Lib.FAILED_ACKNOWLEDGEMENT_JSON,
relayer: makeAddr("relayer")
})
Expand All @@ -474,6 +478,7 @@ contract ICS20TransferTest is Test {
destinationChannel: packet.destChannel,
sequence: packet.sequence,
payload: packet.payloads[0],
recvSuccess: false,
acknowledgement: ICS20Lib.FAILED_ACKNOWLEDGEMENT_JSON,
relayer: makeAddr("relayer")
})
Expand Down Expand Up @@ -773,6 +778,9 @@ contract ICS20TransferTest is Test {

// test invalid version
packet.payloads[0].version = "invalid";
vm.expectRevert(
abi.encodeWithSelector(IICS20Errors.ICS20UnexpectedVersion.selector, ICS20Lib.ICS20_VERSION, "invalid")
);
bytes memory ack = ics20Transfer.onRecvPacket(
IIBCAppCallbacks.OnRecvPacketCallback({
sourceChannel: packet.sourceChannel,
Expand All @@ -782,7 +790,6 @@ contract ICS20TransferTest is Test {
relayer: makeAddr("relayer")
})
);
assertEq(string(ack), "{\"error\":\"unexpected version: invalid\"}");
// Reset version
packet.payloads[0].version = ICS20Lib.ICS20_VERSION;

Expand All @@ -803,6 +810,7 @@ contract ICS20TransferTest is Test {
// test invalid amount
data = ICS20Lib.marshalJSON(ibcDenom, 0, receiverStr, senderStr, "memo");
packet.payloads[0].value = data;
vm.expectRevert(abi.encodeWithSelector(IICS20Errors.ICS20InvalidAmount.selector, 0));
ack = ics20Transfer.onRecvPacket(
IIBCAppCallbacks.OnRecvPacketCallback({
sourceChannel: packet.sourceChannel,
Expand All @@ -812,7 +820,6 @@ contract ICS20TransferTest is Test {
relayer: makeAddr("relayer")
})
);
assertEq(string(ack), "{\"error\":\"invalid amount: 0\"}");

// test receiver chain is source, but denom is not erc20 address
string memory invalidErc20Denom =
Expand All @@ -833,6 +840,7 @@ contract ICS20TransferTest is Test {
// test invalid receiver
data = ICS20Lib.marshalJSON(ibcDenom, defaultAmount, receiverStr, "invalid", "memo");
packet.payloads[0].value = data;
vm.expectRevert(abi.encodeWithSelector(IICS20Errors.ICS20InvalidAddress.selector, "invalid"));
ack = ics20Transfer.onRecvPacket(
IIBCAppCallbacks.OnRecvPacketCallback({
sourceChannel: packet.sourceChannel,
Expand All @@ -842,7 +850,6 @@ contract ICS20TransferTest is Test {
relayer: makeAddr("relayer")
})
);
assertEq(string(ack), "{\"error\":\"invalid receiver: invalid\"}");

// just to document current limitations: JSON needs to be in a very specific order
bytes memory wrongOrderJSON = abi.encodePacked(
Expand Down
4 changes: 2 additions & 2 deletions test/ICS24HostTest.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ contract ICS24HostTest is Test {
bytes memory ack = abi.encodePacked("some bytes");
bytes[] memory acks = new bytes[](1);
acks[0] = ack;
bytes32 ackHash = ICS24Host.packetAcknowledgementCommitmentBytes32(acks);
bytes32 ackHash = ICS24Host.packetAcknowledgementCommitmentBytes32(true, acks);
string memory actualAckHash = Strings.toHexString(uint256(ackHash));
string memory expectedAckHash = "0xf03b4667413e56aaf086663267913e525c442b56fa1af4fa3f3dab9f37044c5b";
string memory expectedAckHash = "0xfc02a4453c297c9b65189ec354f4fc7f0c1327b72f6044a20d4dd1fac8fda9f7";
assertEq(actualAckHash, expectedAckHash);
}

Expand Down
Loading