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.

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

feat: refactor handlers so they return deposit data intead of handler response #15

Merged
merged 5 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 7 additions & 10 deletions src/contracts/Router.sol
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ contract Router is Context {
bytes32 resourceID,
uint64 depositNonce,
address indexed user,
bytes data,
bytes handlerResponse
bytes data
);

modifier onlyAllowed() {
Expand Down Expand Up @@ -80,18 +79,15 @@ contract Router is Context {
@param resourceID ResourceID used to find address of handler to be used for deposit.
@param depositData Additional data to be passed to specified handler.
@param feeData Additional data to be passed to the fee handler.
@notice Emits {Deposit} event with all necessary parameters and a handler response.
@notice Emits {Deposit} event with all necessary parameters.
@return depositNonce deposit nonce for the destination domain.
@return handlerResponse a handler response:
- ERC20Handler: responds with an empty data.
- PermissionlessGenericHandler: responds with an empty data.
*/
function deposit(
uint8 destinationDomainID,
bytes32 resourceID,
bytes calldata depositData,
bytes calldata feeData
) external payable whenBridgeNotPaused returns (uint64 depositNonce, bytes memory handlerResponse) {
) external payable whenBridgeNotPaused returns (uint64 depositNonce) {
if (destinationDomainID == _domainID) revert DepositToCurrentDomain();

address sender = _msgSender();
Expand All @@ -114,10 +110,11 @@ contract Router is Context {

depositNonce = ++_depositCounts[destinationDomainID];


IHandler depositHandler = IHandler(handler);
handlerResponse = depositHandler.deposit(resourceID, sender, depositData);
bytes memory handlerDepositData = depositHandler.deposit(resourceID, sender, depositData);

emit Deposit(destinationDomainID, resourceID, depositNonce, sender, depositData, handlerResponse);
return (depositNonce, handlerResponse);
emit Deposit(destinationDomainID, resourceID, depositNonce, sender, handlerDepositData);
return depositNonce;
}
}
11 changes: 9 additions & 2 deletions src/contracts/handlers/ERC20Handler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, ERC20Safe {
bytes calldata data
) external override onlyBridge returns (bytes memory) {
uint256 amount;
(amount) = abi.decode(data, (uint));
uint256 destinationRecipientAddressLen;
bytes memory destinationRecipientAddress;
(amount, destinationRecipientAddressLen) = abi.decode(data, (uint, uint));
destinationRecipientAddress = bytes(data[64:64 + destinationRecipientAddressLen]);

address tokenAddress = _resourceIDToTokenContractAddress[resourceID];
if (!_tokenContractAddressToTokenProperties[tokenAddress].isWhitelisted)
Expand All @@ -57,7 +60,11 @@ contract ERC20Handler is IHandler, ERCHandlerHelpers, ERC20Safe {
lockERC20(tokenAddress, depositor, address(this), amount);
}

return abi.encodePacked(convertToInternalBalance(tokenAddress, amount));
return abi.encodePacked(
convertToInternalBalance(tokenAddress, amount),
destinationRecipientAddressLen,
destinationRecipientAddress
);
}

/**
Expand Down
6 changes: 3 additions & 3 deletions src/contracts/handlers/ERCHandlerHelpers.sol
Original file line number Diff line number Diff line change
Expand Up @@ -121,17 +121,17 @@ contract ERCHandlerHelpers is IERCHandler {
@param tokenAddress Address of contract to be used when executing proposals.
@param amount Decimals value to be set for {contractAddress}.
*/
function convertToInternalBalance(address tokenAddress, uint256 amount) internal view returns (bytes memory) {
function convertToInternalBalance(address tokenAddress, uint256 amount) internal view returns (uint256) {
Decimals memory decimals = _tokenContractAddressToTokenProperties[tokenAddress].decimals;
uint256 convertedBalance;
if (!decimals.isSet) {
return "";
return amount;
} else if (decimals.externalDecimals >= DEFAULT_DECIMALS) {
convertedBalance = amount / (10 ** (decimals.externalDecimals - DEFAULT_DECIMALS));
} else {
convertedBalance = amount * (10 ** (DEFAULT_DECIMALS - decimals.externalDecimals));
}

return abi.encodePacked(convertedBalance);
return convertedBalance;
}
}
3 changes: 2 additions & 1 deletion src/contracts/handlers/PermissionlessGenericHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@
After this, the target contract will get the following:
executeFuncSignature(address executionDataDepositor, uint[] uintArray, address addr)
*/
function deposit(bytes32 resourceID, address depositor, bytes calldata data) external view returns (bytes memory) {
function deposit(bytes32 resourceID, address depositor, bytes calldata data) external pure returns (bytes memory) {

Check warning on line 112 in src/contracts/handlers/PermissionlessGenericHandler.sol

View workflow job for this annotation

GitHub Actions / Lint Solidity

Variable "resourceID" is unused
require(data.length >= 76, "Incorrect data length"); // 32 + 2 + 1 + 1 + 20 + 20

uint256 maxFee;
Expand Down Expand Up @@ -141,6 +141,7 @@

require(maxFee < MAX_FEE, "requested fee too large");
require(depositor == executionDataDepositor, "incorrect depositor in deposit data");
return data;
}

/**
Expand Down
2 changes: 0 additions & 2 deletions test/contractBridge/depositERC20.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,6 @@ describe("Bridge - [deposit - ERC20]", () => {
expectedDepositNonce,
await depositorAccount.getAddress(),
depositData.toLowerCase(),
"0x",
);

const depositTx2 = routerInstance
Expand All @@ -203,7 +202,6 @@ describe("Bridge - [deposit - ERC20]", () => {
expectedDepositNonce + 1,
await depositorAccount.getAddress(),
depositData.toLowerCase(),
"0x",
);
});

Expand Down
35 changes: 10 additions & 25 deletions test/e2e/erc20/decimals/bothChainsNot18Decimals.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
deployBridgeContracts,
createResourceID,
createERCDepositData,
createDepositProposalDataFromHandlerResponse,
toHex,
} from "../../../helpers";
import type {
Expand Down Expand Up @@ -206,6 +205,9 @@ describe("E2E ERC20 - Two EVM Chains both with decimal places != 18", () => {

await expect(originDepositTx).not.to.be.reverted;

const originExpectedDepositData =
toHex(relayerConvertedAmount.toString(), 32) +
originDepositData.substring(66);
// check that deposited amount converted to 18 decimal places is
// emitted in handlerResponse
await expect(originDepositTx)
Expand All @@ -215,23 +217,13 @@ describe("E2E ERC20 - Two EVM Chains both with decimal places != 18", () => {
originResourceID.toLowerCase(),
expectedDepositNonce,
await depositorAccount.getAddress(),
originDepositData.toLowerCase(),
toHex(relayerConvertedAmount.toString(), 32),
);

// this mocks depositProposal data for executing on
// destination chain which is returned from relayers
const originDepositProposalData =
await createDepositProposalDataFromHandlerResponse(
originDepositTx,
20,
await recipientAccount.getAddress(),
originExpectedDepositData.toLowerCase(),
);

const originDomainProposal = {
originDomainID: originDomainID,
depositNonce: expectedDepositNonce,
data: originDepositProposalData,
data: originExpectedDepositData,
resourceID: destinationResourceID,
};

Expand Down Expand Up @@ -282,6 +274,9 @@ describe("E2E ERC20 - Two EVM Chains both with decimal places != 18", () => {
);
await expect(destinationDepositTx).not.to.be.reverted;

const destinationExepectedDepositData =
toHex(relayerConvertedAmount.toString(), 32) +
destinationDepositData.substring(66);
// check that deposited amount converted to 18 decimal places is
// emitted in handlerResponse
await expect(destinationDepositTx)
Expand All @@ -291,23 +286,13 @@ describe("E2E ERC20 - Two EVM Chains both with decimal places != 18", () => {
destinationResourceID.toLowerCase(),
expectedDepositNonce,
await recipientAccount.getAddress(),
destinationDepositData.toLowerCase(),
toHex(relayerConvertedAmount.toString(), 32),
);

// this mocks depositProposal data for executing on
// destination chain which is returned from relayers
const destinationDepositProposalData =
await createDepositProposalDataFromHandlerResponse(
destinationDepositTx,
20,
await depositorAccount.getAddress(),
destinationExepectedDepositData.toLowerCase(),
);

const destinationDomainProposal = {
originDomainID: destinationDomainID,
depositNonce: expectedDepositNonce,
data: destinationDepositProposalData,
data: destinationExepectedDepositData,
resourceID: originResourceID,
};

Expand Down
14 changes: 2 additions & 12 deletions test/e2e/erc20/decimals/oneChainNot18Decimals.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
deployBridgeContracts,
createResourceID,
createERCDepositData,
createDepositProposalDataFromHandlerResponse,
getDepositEventData,
} from "../../../helpers";
import type {
Bridge,
Expand Down Expand Up @@ -224,19 +224,10 @@ describe("E2E ERC20 - Two EVM Chains, one with decimal places == 18, other with
);
await expect(originDepositTx).not.to.be.reverted;

// this mocks depositProposal data for executing on
// destination chain which is returned from relayers
const originDepositProposalData =
await createDepositProposalDataFromHandlerResponse(
originDepositTx,
20,
await recipientAccount.getAddress(),
);

const originDomainProposal = {
originDomainID: originDomainID,
depositNonce: expectedDepositNonce,
data: originDepositProposalData,
data: await getDepositEventData(originDepositTx),
resourceID: destinationResourceID,
};

Expand Down Expand Up @@ -297,7 +288,6 @@ describe("E2E ERC20 - Two EVM Chains, one with decimal places == 18, other with
expectedDepositNonce,
await recipientAccount.getAddress(),
destinationDepositData.toLowerCase(),
"0x",
);

// Recipient should have a balance of 0 (deposit amount)
Expand Down
14 changes: 2 additions & 12 deletions test/e2e/erc20/decimals/oneChainWith0Decimals.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
deployBridgeContracts,
createResourceID,
createERCDepositData,
createDepositProposalDataFromHandlerResponse,
getDepositEventData,
} from "../../../helpers";
import type {
Bridge,
Expand Down Expand Up @@ -226,19 +226,10 @@ describe("E2E ERC20 - Two EVM Chains, one with decimal places == 18, other with

await expect(originDepositTx).not.to.be.reverted;

// this mocks depositProposal data for executing on
// destination chain which is returned from relayers
const originDepositProposalData =
await createDepositProposalDataFromHandlerResponse(
originDepositTx,
20,
await recipientAccount.getAddress(),
);

const originDomainProposal = {
originDomainID: originDomainID,
depositNonce: expectedDepositNonce,
data: originDepositProposalData,
data: getDepositEventData(originDepositTx),
resourceID: destinationResourceID,
};

Expand Down Expand Up @@ -299,7 +290,6 @@ describe("E2E ERC20 - Two EVM Chains, one with decimal places == 18, other with
expectedDepositNonce,
await recipientAccount.getAddress(),
destinationDepositData.toLowerCase(),
"0x",
);

// Recipient should have a balance of 0 (deposit amount)
Expand Down
42 changes: 13 additions & 29 deletions test/e2e/erc20/decimals/roundingLoss.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
createResourceID,
createERCDepositData,
toHex,
createDepositProposalDataFromHandlerResponse,
getDepositEventData,
} from "../../../helpers";
import type {
Bridge,
Expand Down Expand Up @@ -216,32 +216,24 @@ describe("E2E ERC20 - Two EVM Chains both with decimal places != 18 with roundin
);
await expect(originDepositTx).not.to.be.reverted;

// check that deposited amount converted to 18 decimal places is
// emitted in handlerResponse
// check that deposited amount converted to 18 decimal places is emitted in data
const originExpectedDepositAmount =
toHex(originRelayerConvertedAmount.toString(), 32) +
originDepositData.substring(66);
await expect(originDepositTx)
.to.emit(originRouterInstance, "Deposit")
.withArgs(
destinationDomainID,
originResourceID.toLowerCase(),
expectedDepositNonce,
await depositorAccount.getAddress(),
originDepositData.toLowerCase(),
toHex(originRelayerConvertedAmount.toString(), 32),
);

// this mocks depositProposal data for executing on
// destination chain which is returned from relayers
const originDepositProposalData =
await createDepositProposalDataFromHandlerResponse(
originDepositTx,
20,
await recipientAccount.getAddress(),
originExpectedDepositAmount.toLowerCase(),
);

const originDomainProposal = {
originDomainID: originDomainID,
depositNonce: expectedDepositNonce,
data: originDepositProposalData,
data: getDepositEventData(originDepositTx),
resourceID: destinationResourceID,
};

Expand Down Expand Up @@ -292,32 +284,24 @@ describe("E2E ERC20 - Two EVM Chains both with decimal places != 18 with roundin
);
await expect(destinationDepositTx).not.to.be.reverted;

// check that deposited amount converted to 18 decimal places is
// emitted in handlerResponse
// check that deposited amount converted to 18 decimal places is emitted in data
const destinationExpectedDepositData =
toHex(destinationRelayerConvertedAmount.toString(), 32) +
destinationDepositData.substring(66);
await expect(destinationDepositTx)
.to.emit(destinationRouterInstance, "Deposit")
.withArgs(
originDomainID,
destinationResourceID,
expectedDepositNonce,
await recipientAccount.getAddress(),
destinationDepositData.toLowerCase(),
toHex(destinationRelayerConvertedAmount.toString(), 32),
);

// this mocks depositProposal data for executing on
// destination chain which is returned from relayers
const destinationDepositProposalData =
await createDepositProposalDataFromHandlerResponse(
destinationDepositTx,
20,
await depositorAccount.getAddress(),
destinationExpectedDepositData.toLowerCase(),
);

const destinationDomainProposal = {
originDomainID: destinationDomainID,
depositNonce: expectedDepositNonce,
data: destinationDepositProposalData,
data: await getDepositEventData(destinationDepositTx),
resourceID: originResourceID,
};

Expand Down
2 changes: 0 additions & 2 deletions test/handlers/erc20/deposit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ describe("ERC20Handler - [Deposit ERC20]", () => {
lenRecipientAddress,
recipientAccount,
).toLowerCase(),
"0x",
);
});

Expand Down Expand Up @@ -151,7 +150,6 @@ describe("ERC20Handler - [Deposit ERC20]", () => {
lenRecipientAddress,
recipientAccount,
),
"0x",
);
});

Expand Down
2 changes: 0 additions & 2 deletions test/handlers/fee/basic/collectFee.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ describe("BasicFeeHandler - [collectFee]", () => {
expectedDepositNonce,
await depositorAccount.getAddress(),
erc20depositData.toLowerCase(),
"0x",
);

await expect(depositTx)
Expand Down Expand Up @@ -415,7 +414,6 @@ describe("BasicFeeHandler - [collectFee]", () => {
expectedDepositNonce,
await depositorAccount.getAddress(),
erc20depositData.toLowerCase(),
"0x",
);

await expect(depositTx)
Expand Down
Loading
Loading