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: add domain routing hook #38

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Xavek
Copy link

@Xavek Xavek commented Jun 17, 2024

Implements #29

Copy link
Contributor

@EvolveArt EvolveArt left a comment

Choose a reason for hiding this comment

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

smol comment will let @JordyRo1 give a detailled review

contracts/src/interfaces.cairo Outdated Show resolved Hide resolved
contracts/src/interfaces.cairo Outdated Show resolved Hide resolved
@JordyRo1
Copy link
Contributor

JordyRo1 commented Jun 18, 2024

Hi, can you add some test coverage for your contract, please ? You can deploy your contract by defining a function in the setup.cairo (cf: https://github.com/astraly-labs/hyperlane_starknet/blob/main/contracts/src/tests/setup.cairo). You can use setup_mock_hook to deploy a mock hook for your testing. Regarding the code you already wrote, everything looks good at first sight, nice job !

@EvolveArt EvolveArt changed the title add domain routing hook feat: add domain routing hook Jun 18, 2024
@Xavek
Copy link
Author

Xavek commented Jun 20, 2024

@JordyRo1 Tried to add tests. Can you have a look now

@Xavek Xavek requested a review from EvolveArt June 20, 2024 11:46
@EvolveArt EvolveArt requested a review from JordyRo1 June 20, 2024 12:41
}

fn post_dispatch(ref self: ContractState, _metadata: Bytes, _message: Message) {
self._get_configured_hook(_message.clone()).post_dispatch(_metadata, _message);
Copy link
Contributor

Choose a reason for hiding this comment

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

The post dispatch function lacks the fee mecanism (represented on the solidity version by msg.value). Since we do not have native token, we will have to transfer erc20 manually. Leave it as it is right, now, I will handle it.

Copy link
Author

@Xavek Xavek Jun 24, 2024

Choose a reason for hiding this comment

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

I can try to add the fee mechanism. Context would be the protocol_fee hook? Additionally, did you have a look at my test files

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, protocol fee hook is the context and since it is a routing hook, it will require to quote_dispatch.

let ownable = IOwnableDispatcher { contract_address: set_routing_hook_addrs.contract_address };
start_prank(CheatTarget::One(ownable.contract_address), OWNER());
let destination: u32 = 18;
let hook: ContractAddress = setup_mock_hook().contract_address;
Copy link
Contributor

Choose a reason for hiding this comment

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

The errror Result: unwrap failed comes from this setup_mock_hook. Since we already declared the setup_mock_hook for the mailbox setup, calling it again here cause the declaration to fail because the contract is already declared

@Xavek Xavek requested a review from JordyRo1 June 26, 2024 08:18
let token_dispatcher = IERC20Dispatcher { contract_address: self.fee_token.read() };
let user_balance = token_dispatcher.balance_of(from);
assert(user_balance >= amount, Errors::INSUFFICIENT_BALANCE);
let transfer_flag: bool = token_dispatcher.transfer_from(from, to, amount);
Copy link
Contributor

@JordyRo1 JordyRo1 Jun 27, 2024

Choose a reason for hiding this comment

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

Can you check the allowance for the caller before the transfer

@JordyRo1
Copy link
Contributor

Reviewed. The general hook idea is understood, everything seems fine !

@Xavek Xavek requested a review from JordyRo1 July 1, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants