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

Polygon ZKevm hook and ism #3136

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

curlypoint
Copy link

@curlypoint curlypoint commented Jan 8, 2024

Description

This PR includes the hook and ISM contract for polygon's zkEVM chain.

The contracts utilise the polygon zkevm bridge contracts to make sure the origin of the cross chain message is trusted, this relies on the fact that zkevm bridge uses exit nodes to verify the integrity.

more details can be found here

PR for docs : PR#40

Drive-by changes

  • nothing as of now

Related issues

Backward compatibility

Yes

Testing

🚧 Work in progress 🚧

Copy link

changeset-bot bot commented Jan 8, 2024

⚠️ No Changeset found

Latest commit: c4130b5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@curlypoint
Copy link
Author

curlypoint commented Jan 9, 2024

it seems they require an off chain compute server to get the smt proof and submit the claim transaction on the destination chain.

they do have their own server that provides the proof needed see this , or we can host our own using this

where should I write the script that's needed by the validators running for zkevm L1 and L2, and which path should I go ahead with? (edited)

@yorhodes
Copy link
Member

it seems they require an off chain compute server to get the smt proof and submit the claim transaction on the destination chain.

You can use the CCIP Read ISM to do the offchain lookup

@curlypoint
Copy link
Author

https://github.com/curlypoint/hyperlane-zkevm-ccip

Hey sorry for the confusion, I was talking about this. for now I've made a repo on my own account, we can move this to hyperlane repo once I figure where to put it

@curlypoint curlypoint marked this pull request as ready for review January 15, 2024 18:53
@curlypoint
Copy link
Author

also, Having troubles in the testcases, might need some help there.

I also need help with the CCIP server implementation, where should I include those files in the PR.

meanwhile, if someone can just do a little sanity check with the PR, that'd be amazing too.

Copy link
Member

@yorhodes yorhodes left a comment

Choose a reason for hiding this comment

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

its a good start
I havent taken a look at the server implementation yet
I was hoping we dont need a separate server for every CCIP ISM

IPolygonZkEVMBridge public immutable zkEvmBridge;

// left-padded address for ISM to verify messages
bytes32 public immutable ism;
Copy link
Member

Choose a reason for hiding this comment

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

because we know this is always an EVM destination, this can be address

Comment on lines 65 to 67
destinationDomain = _destinationDomain;
zkEvmBridge = IPolygonZkEVMBridge(_zkEvmBridge);
zkBridgeChainIdDestination = uint8(_zkBridgeChainId);
Copy link
Member

Choose a reason for hiding this comment

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

is the bridge chain ID different from destination domain?

Copy link
Author

@curlypoint curlypoint Jan 18, 2024

Choose a reason for hiding this comment

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

Ahh yes, Polygon Zkevm Bridge uses ID 0 and 1 for Eth-mainnet and rollup chain respectively

Copy link
Member

Choose a reason for hiding this comment

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

in general prefer not to vendor these
can we import this somehow?

Copy link
Author

Choose a reason for hiding this comment

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

I only found this repository for the contracts, I could not find the contracts in any npm package.

return true;
}

/// @dev This value is hardcoded to 0 because the Polygon zkEVM bridge does not support fee quotes
Copy link
Member

Choose a reason for hiding this comment

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

how are fees calculated?

Copy link
Author

@curlypoint curlypoint Jan 18, 2024

Choose a reason for hiding this comment

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

Users have to call the claim function on destination chain, we're calling that in verify(metadata,message) call,after fetching the smt proof needed using CCIP.

solidity/contracts/hooks/PolygonZkevmHook.sol Show resolved Hide resolved
);
bytes32 messageId = message.id();

zkEvmBridge.bridgeMessage(
Copy link
Member

Choose a reason for hiding this comment

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

can you pass value {value: metadata.msgValue(0)} here?

solidity/contracts/isms/hook/PolygonZkevmIsm.sol Outdated Show resolved Hide resolved
*/
function verify(
bytes calldata _metadata,
bytes calldata
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bytes calldata
bytes calldata message

you need to compare message.id() to the payload you passed on the origin chain

* @inheritdoc AbstractMessageIdAuthorizedIsm
*/
function _isAuthorized() internal view override returns (bool) {
bytes32 originSender = abi.decode(msg.data[4:], (bytes32));
Copy link
Member

Choose a reason for hiding this comment

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

I dont really understand this

Copy link
Member

Choose a reason for hiding this comment

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

you need to compare the address originSender from onMessageReceived to the authorizedHook

Copy link
Author

Choose a reason for hiding this comment

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

Umm okay, So I wanted to use the _isAuthorized() for the verification, tho now I feel like just using a normal require check for originSender == TypeCasts.bytes32ToAddress(authorizedHook) would be better.

I was just slicing the function selector and just decoding the address to match it with authorisedHook

* Verifies the received message.
* @inheritdoc IBridgeMessageReceiver
*/
function onMessageReceived(
Copy link
Member

Choose a reason for hiding this comment

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

anyone can call this, we need to restrict it to the zkevmbridge address?

@@ -74,7 +74,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is
bytes calldata,
/*_metadata*/
bytes calldata message
) external returns (bool) {
) external virtual returns (bool) {
Copy link
Author

Choose a reason for hiding this comment

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

Needed to modify this function as relayer caller verify after the CCIP call

originNetwork,
originAddress,
zkEvmBridgeDestinationNetId,
address(this),
Copy link
Author

Choose a reason for hiding this comment

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

was running into stack too deep, this should revert if does not match.

@avious00
Copy link
Contributor

hey @curlypoint let me know when Yorke's comments are resolved and you'd like another review. thanks for contributing!

@curlypoint
Copy link
Author

Hey @avious00 , sorry for a delayed response, I made the requested changes, but I'm out of luck writing testcases on foundry. I need some help with that, rest of the PR requirements are fulfilled though :)

@avious00
Copy link
Contributor

avious00 commented Mar 5, 2024

sorry for missing your message @curlypoint - @aroralanuk can you share some advice or references here that could help them?

@iamzubin
Copy link

I'd like to continue the work on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Polygon zkEVM Hooks and ISMs
4 participants