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

Add custom error to CrossChainEnabledPolygonChild #3380

Merged
merged 6 commits into from
Apr 29, 2022
Merged

Add custom error to CrossChainEnabledPolygonChild #3380

merged 6 commits into from
Apr 29, 2022

Conversation

pcaversaccio
Copy link
Contributor

@pcaversaccio pcaversaccio commented Apr 29, 2022

In order to preserve custom error consistency within the crosschain contracts, I introduce a new custom error UnauthorizedCrossChainRelayer used within CrossChainEnabledPolygonChild. The reason why I introduce a new custom error is that I don't want to misuse the existing custom error InvalidCrossChainSender and make it more specific to the exact error message.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

Signed-off-by: Pascal Marco Caversaccio <[email protected]>
Signed-off-by: Pascal Marco Caversaccio <[email protected]>
@Amxx
Copy link
Collaborator

Amxx commented Apr 29, 2022

Hello @pcaversaccio and thank you for raising that interesting point.

I realize that the
require(msg.sender == _fxChild, "unauthorized cross-chain relay");
Is in fact equivalent to the other implementation's
if (!isCrossChain(bridge)) revert NotCrossChainCall();

This makes me think we should probably not introduce a new error and do

    function processMessageFromRoot(
        uint256, /* stateId */
        address rootMessageSender,
        bytes calldata data
    ) external override nonReentrant {
        if (!_isCrossChain()) revert NotCrossChainCall();

        _sender = rootMessageSender;
        Address.functionDelegateCall(address(this), data, "crosschain execution failled");
        _sender = DEFAULT_SENDER;
    }

What do you think ?

Signed-off-by: Pascal Marco Caversaccio <[email protected]>
@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Apr 29, 2022

@Amxx good point - fully agreed; will fix this now! Quick suggestion: why not include the address of the emitter in all custom errors (i.e. here). This helps end-users identify which contract was reverted with a failed transaction, which is especially useful for complex transactions involving multiple contracts? What do you think?

error SomeError(address emitter, ..., ..., ...);
emit SomeError(address(this), ..., ..., ...);

PS: Just fixed a typo in Address.functionDelegateCall(address(this), data, "cross-chain execution failed");.

Signed-off-by: Pascal Marco Caversaccio <[email protected]>
Signed-off-by: Pascal Marco Caversaccio <[email protected]>
@Amxx
Copy link
Collaborator

Amxx commented Apr 29, 2022

I fully agree that custom errors should start with the address of the emitter.

@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Apr 29, 2022

Ok - so let's first get this PR merged (if you agree) and thereafter I will open another PR with the custom errors that include the emitter address. How would you think about creating a dedicated error interface ICrossChainErrors (instead of the current errors.sol), where we define all the custom errors used in crosschain. This design could be later extended for all the other OZ contracts (e.g. ERC20 etc.) once solc 0.8.4 is supported.

@Amxx
Copy link
Collaborator

Amxx commented Apr 29, 2022

Ok - so let's first get this PR merged (if you agree) and thereafter I will open another PR with the custom errors that include the emitter address.
100% agree

How would you think about creating a dedicated error interface ICrossChainErrors (instead of the current errors.sol), where we define all the custom errors used in crosschain. This design could be later extended for all the other OZ contracts (e.g. ERC20 etc.) once solc 0.8.4 is supported.

I'm not sure, we'll have to discuss that internally.

@Amxx Amxx merged commit be3cfa0 into OpenZeppelin:master Apr 29, 2022
@pcaversaccio
Copy link
Contributor Author

Sure, will work on the other PR in the meantime.

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.

2 participants