Skip to content

Add custom error to CrossChainEnabledPolygonChild #3380

Merged
Amxx merged 6 commits into
OpenZeppelin:masterfrom
pcaversaccio:master
Apr 29, 2022
Merged

Add custom error to CrossChainEnabledPolygonChild #3380
Amxx merged 6 commits into
OpenZeppelin:masterfrom
pcaversaccio:master

Conversation

@pcaversaccio

@pcaversaccio pcaversaccio commented Apr 29, 2022

Copy link
Copy Markdown
Contributor

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 <pascal.caversaccio@hotmail.ch>
Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
@Amxx

Amxx commented Apr 29, 2022

Copy link
Copy Markdown
Collaborator

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 <pascal.caversaccio@hotmail.ch>
@pcaversaccio

pcaversaccio commented Apr 29, 2022

Copy link
Copy Markdown
Contributor Author

@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 <pascal.caversaccio@hotmail.ch>
Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
@Amxx

Amxx commented Apr 29, 2022

Copy link
Copy Markdown
Collaborator

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

@pcaversaccio

pcaversaccio commented Apr 29, 2022

Copy link
Copy Markdown
Contributor Author

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

Amxx commented Apr 29, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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