Skip to content

Conversation

@chrismaree
Copy link
Member

@chrismaree chrismaree commented Mar 16, 2022

Problem:

We have identified some areas of the code which could benefit from better naming:

  • In HubPoolInterface.liquidityUtilizationPostRelay, the parameter token should be
    renamed to l1Token to better match other functions in the interface, as well as the function's
    implementation in HubPool.
  • In the RootBundle struct, requestExpirationTimestamp should be renamed to better indicate
    that it ends the "challenge period". Consider renaming it to ChallengePeriodEndTimestamp or
    similar.
  • The RootBundleExecuted event in HubPool.sol only names one of its array parameters in
    the plural form, but when the event is emitted, all array parameters are named in the plural
    form. Consider changing the event definition so that all array parameters are pluralized.
  • The name of function whitelistedRoute is vague and does not indicate what it's output will
    be. Consider renaming it to something like destinationTokenFromRoute to better match the
    return value.
  • When weth is used in Polygon_SpokePool.sol, it refers to wrapped MATIC. Consider renaming
    the weth variable in SpokePool.sol to wrapped_native_token to make it more generalizable.
  • This will make Polygon_SpokePool less confusing and be more generalizeable for future
    SpokePools.
  • The executeSlowRelayRoot and executeRelayerRefundRoot functions are executing leaves
    and should be renamed accordingly.
  • The unclaimedPoolRebalanceLeafCount parameter of the ProposeRootBundle event should be
    renamed to poolRebalanceLeafCount, since it's always the total number of leaves in the tree.
  • The RootBundleCanceled event names the last parameter as disputedAncillaryData, but the
    proposal is not necessarily disputed. It should just be ancillaryData.
  • The _append function of the LpTokenFactory could be called _concatenate to better describe
    its functionality.
  • The onlyEnabledRoute modifier has a destinationId parameter that should be
    destinationChainId to match the rest of the code base.

Solution:

all recommendations have been made.

Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
@chrismaree chrismaree marked this pull request as draft March 16, 2022 07:14
@chrismaree
Copy link
Member Author

Note that this PR requires a few others to go in first that it is dependent on to avoid conflicts. In particular:

@chrismaree chrismaree requested a review from nicholaspai March 16, 2022 07:16
WETH9 public weth;
// Address of wrappedNativeToken contract for this network. If an origin token matches this, then the caller can
// optionally instruct this contract to wrap native tokens when depositing (ie ETH->WETH or MATIC->WMATIC).
WETH9 public wrappedNativeToken;
Copy link
Member Author

Choose a reason for hiding this comment

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

this change is potentially contentious. I'm curious what you think.


/********************************************************
* ARBITRUM-SPECIFIC CROSS-CHAIN ADMIN FUNCTIONS *
* POLYGON-SPECIFIC CROSS-CHAIN ADMIN FUNCTIONS *
Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated but saw this comment issue.

@chrismaree chrismaree added the OZ Audit - March Resolves issue discovered in March 2022 OZ Audit label Mar 16, 2022
@mrice32 mrice32 self-requested a review March 17, 2022 12:28
Signed-off-by: chrismaree <christopher.maree@gmail.com>
using SafeERC20 for IERC20;
using Address for address;

// A data worker can optimistically store several merkle roots on this contract by staking a bond and calling
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why it shows this diff here...previous PR moved this around.

@chrismaree chrismaree marked this pull request as ready for review March 18, 2022 06:35
event RootBundleDisputed(address indexed disputer, uint256 requestTime, bytes disputedAncillaryData);

event RootBundleCanceled(address indexed disputer, uint256 requestTime, bytes disputedAncillaryData);
event RootBundleCanceled(address indexed disputer, uint256 requestTime, bytes ancillaryData);
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this change since ancillaryData is no longer included in this event following #114

Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

This looks awesome but rebase master before you merge and check on any merge conflicts

Signed-off-by: chrismaree <christopher.maree@gmail.com>
@chrismaree chrismaree merged commit 729a9b8 into master Mar 18, 2022
@chrismaree chrismaree deleted the chrismaree/n08 branch March 18, 2022 16:14
nicholaspai pushed a commit that referenced this pull request Mar 21, 2022
* fix[N02} Move all structs to the same place

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* fix[N03] Fixed inconsistant token metadata versioning

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* nit

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* nit

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* fix[N08] Propose fixes to some naming issues

Signed-off-by: chrismaree <christopher.maree@gmail.com>
nicholaspai added a commit that referenced this pull request Mar 22, 2022
)

* improve: Update solc version to 8.13

* feat: Changes after running 8.13 wargames

* Update Optimism_Adapter.json

* Init commit: arbitrum rinkeby

* finish arbitrum war games

* fix[N06] Fix misleading comments (#104)

* fix[N04] Add additional documentation

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* nit

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* fix[N06] Fix missleading comments

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* Apply suggestions from code review

Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>

* nit

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>

* fix[N08] Propose fixes to some naming issues (#105)

* fix[N02} Move all structs to the same place

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* fix[N03] Fixed inconsistant token metadata versioning

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* nit

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* nit

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* fix[N08] Propose fixes to some naming issues

Signed-off-by: chrismaree <christopher.maree@gmail.com>

* fix[L04] Enforce chainId requirements in PolygonTokenBridger (#115)

* fix: Arbitrum Adapter needs to pay for L2 gas

* Add helper scripts

* Update Arbitrum_Adapter.sol

Co-authored-by: Chris Maree <christopher.maree@gmail.com>
Co-authored-by: Matt Rice <matthewcrice32@gmail.com>
This was referenced May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OZ Audit - March Resolves issue discovered in March 2022 OZ Audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants