fix: polygon testnet fixes #128

mrice32 · 2022-03-25T20:04:15Z

This PR includes three changes related to testnet testing against mumbai:

The PolygonTokenBridger's receive function needs to be gas-minimized because the polygon contracts provide minimal gas when sending ETH to it.
The approval in the Polygon adapter must be for the erc20 predicate, not the root chain manager that it directly calls.
The SpokePool must wrap matic that it receives from the plasma bridge.
The PolygonTokenBridger needs to be able to call startExitWithBurntTokens with custom data to complete the L1 exit when withdrawing matic via the plasma bridge on L1.
The Polygon Adapter contract must be able to send matic via the plasma bridge rather than the normal PoS bridge.

Note: the bi-directional transfer of ERC20s, WETH, and Matic has been tested on mumbai.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai

Can you add a PR description explaining why we want to remove wrapping from the fallback functikn? LGTM otherwise I think allowing anyone to wrap this contracts ETH makes sense

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai · 2022-04-04T15:27:48Z

contracts/PolygonTokenBridger.sol

     * @notice Constructs Token Bridger contract.
     * @param _destination Where to send tokens to for this network.
-     * @param _l1Weth Ethereum WETH address.
+     * @param _polygonRegistry L1 registry that stores updated addresses of polygon contracts.


Can you add a comment explaining how to set this on L2 since its described as an "L1 registry"

Sure. Can do. It should still be the L1 address to ensure that this gets deployed at an identical address on both chains.

nicholaspai · 2022-04-04T15:29:25Z

contracts/Polygon_SpokePool.sol

+     * @dev Matic sends via L1 -> L2 bridging actions don't call into the contract receiving the tokens, so wrapping
+     * must be done via a separate transaction.
+     */
+    function wrap() public nonReentrant {


Should wrap be triggered on any other external functions like processMessageFromRoot for convenience?

It could. Note: the functions that actually would need the tokens aren't triggered by processMessageFromRoot, though. Executing relayer refunds and slow relays are the ones that would require this, but to add it to those, they might need to be overridden.

Will think a little about how to do this with little overhead.

Added this functionality in the last commit: 811ac20. PTAL.

Ah perfect this is now consistent with how Optimism_SpokePool handles the same logic: https://github.com/across-protocol/contracts-v2/blob/master/contracts/Optimism_SpokePool.sol#L89

HOWEVER, you can probably save some gas by adding a check if (destinationToken == weth) ...

I don't really think this extra check is worthwhile because:

We already check address(this).balance > 0, which is extremely cheap. It costs 5 gas according to https://ethereum.org/en/developers/docs/evm/opcodes/ to read the balance. Reading the weth value from the contract, by comparison, would cost 100 - 2100 gas. I think these checks are the only difference between the two, since we will likely run some matic execution step each time matic tokens are sent over to the spoke, meaning the number of wraps shouldn't really differ between the two.

Since this is on polygon, gas costs shouldn't really matter too much.

As discussed offline, we'll leave this as is.

nicholaspai

LGTM, left some comments just requesting more detailed comments

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai

I left one suggestion on the use of _wrap but overall this looks good as before!

ericDeCourcy · 2022-04-21T22:22:01Z

contracts/test/PolygonMocks.sol

+
+contract PolygonRegistryMock {
+    // solhint-disable-next-line no-empty-blocks
+    function erc20Predicate() external returns (address predicate) {}


I know this is a mock, but it looks like using the "registry" from polygon's list may give the wrong value. The Registry is 0x33a02E6cC863D393d6Bf231B697b82F6e499cA71, and you can see that function erc20Predicate returns 0x158d5fa3Ef8e4dDA8a5367deCF76b94E7efFCe95. Just based on the tx's i see calling that contract, it's for exiting from Matic, but not bridging/depositing onto matic.

Again since this is a mock, you can have it return whatever value you want. So not a problem. But it might lead to someone thinking that the actual mainnet contract will return the "deposit" predicate, which it doesn't. I believe the deposit predicate is 0x40ec5b33f54e0e8a33a975908c5ba1c14e5bbbdf.

I'm thinking that Polygon_Adapter.sol is deployed on ETH mainnet, for transferring tokens over to Matic. If that's not true... this doesn't apply

fix: make receive function in PolygonTokenBridger gas-minimized

330de58

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested review from chrismaree and nicholaspai March 25, 2022 20:04

nicholaspai approved these changes Mar 25, 2022

View reviewed changes

mrice32 added 2 commits March 25, 2022 16:26

WIP

c65505a

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

ceab201

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 changed the title ~~fix: make receive function in PolygonTokenBridger gas-minimized~~ fix: polygon war games fixes Mar 28, 2022

WIP

bbb326c

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 changed the title ~~fix: polygon war games fixes~~ fix: polygon testnet fixes Mar 28, 2022

chrismaree approved these changes Mar 28, 2022

View reviewed changes

mrice32 added 6 commits March 31, 2022 23:46

WIP

9dcdd7f

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

Merge branch 'master' into polygon_token_bridger

8449857

WIP

2ca5759

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

update deployments

0a7d597

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

3a10bb9

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

9b0098d

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

nicholaspai reviewed Apr 4, 2022

View reviewed changes

nicholaspai approved these changes Apr 4, 2022

View reviewed changes

mrice32 added 5 commits April 4, 2022 17:57

add tests

5058922

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

32f06d5

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

6f0d0e5

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

baaa78e

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

WIP

811ac20

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>

mrice32 requested review from chrismaree and nicholaspai April 5, 2022 00:15

nicholaspai approved these changes Apr 5, 2022

View reviewed changes

mrice32 requested a review from nicholaspai April 5, 2022 15:34

mrice32 merged commit 72b796e into master Apr 5, 2022

mrice32 deleted the polygon_token_bridger branch April 5, 2022 15:35

chrismaree mentioned this pull request Apr 8, 2022

feat: add rate model store on rinkeby #132

Merged

chrismaree mentioned this pull request Apr 20, 2022

chrismaree/add block number to export #143

Merged

ericDeCourcy reviewed Apr 21, 2022

View reviewed changes

This was referenced May 3, 2022

chrismaree/mainnet deployment #149

Merged

Latest kovan deployment #154

Merged

feat: add reverse look up for contract info from address #155

Merged

chrismaree mentioned this pull request May 10, 2022

chrisamree/mumbai deploy #157

Merged

This was referenced May 17, 2022

fix: address issue with weth on boba #159

Merged

fix: add haircut proposal #160

Merged

fix: address param ordering issue #163

Merged

fix: polygon testnet fixes #128

fix: polygon testnet fixes #128

Uh oh!

Conversation

mrice32 commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mrice32 commented Mar 25, 2022 •

edited

Loading

mrice32 Apr 4, 2022 •

edited

Loading