fix: Response to audit of #3188 and #3089 #3208
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicholaspai
commented
Jul 8, 2021
| @@ -16,7 +16,7 @@ contract GovernorChildTunnel is FxBaseChildTunnel { | |||
| * @dev The data will be received automatically from the state receiver when the state is synced between Ethereum | |||
| * and Polygon. This will revert if the Root chain sender is not the `fxRootTunnel` contract. | |||
| * @param sender The sender of `data` from the Root chain. | |||
| * @param data ABI encoded params with which to call `_publishPrice`. | |||
| * @param data ABI encoded params to include in delegated transaction. | |||
There was a problem hiding this comment.
Pull Request 3089
This PR modifies the chainbridge SourceGovernor and SinkGovernor contracts so the cross-chain messages cannot specify an ETH amount. We don't have any comments.
It also implements the Polygon Tunnel for governance actions. In particular, it introduces GovernorRootTunnel and GovernorChildTunnel. Our only comment is that line 19 of GovernorChildTunnel.sol references a non-existent _publishPrice function.
nicholaspai
commented
Jul 8, 2021
| @@ -98,7 +98,7 @@ library AncillaryData { | |||
|
|
|||
| /** | |||
| * @notice Helper method that returns the left hand side of a "key:value" pair plus the colon ":" and a leading | |||
| * comma "," if the `currentAncillaryData` is not empty. The return value is intended to be appended as a prefix to | |||
| * comma "," if the `currentAncillaryData` is not empty. The return value is intended to be prepended as a prefix to | |||
There was a problem hiding this comment.
Pull Request 3188
This PR addresses the comments from our review of PR 3061 and our batch review of PRs 3054, 3082, and 3092. We have the following comment:
- The comment explaining the
constructPrefixfunction says "appended as a prefix" instead of "prepended".
mrice32
approved these changes
Jul 8, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Pull Request 3188
This PR addresses the comments from our review of PR 3061 and our batch review of PRs 3054, 3082, and 3092. We have the following comment:
constructPrefixfunction says "appended as a prefix" instead of "prepended".Pull Request 3089
This PR modifies the chainbridge
SourceGovernorandSinkGovernorcontracts so the cross-chain messages cannot specify an ETH amount. We don't have any comments.It also implements the Polygon Tunnel for governance actions. In particular, it introduces
GovernorRootTunnelandGovernorChildTunnel. Our only comment is that line 19 ofGovernorChildTunnel.solreferences a non-existent_publishPricefunction.