Skip to content

Conversation

@mrice32
Copy link
Contributor

@mrice32 mrice32 commented Feb 25, 2022

No description provided.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
bytes32 slowRelayRoot
) public override nonReentrant noActiveRequests {
// Note: this is to prevent "empty block" style attacks where someone can make empty proposals that are
// technically valid but not useful. This could also potentially be enforced at the UMIP-level.
Copy link
Member

Choose a reason for hiding this comment

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

Do you think there's a situation where we want to publish a new relayer refund root or slow relay root to the SpokePool but we don't want to execute a pool rebalance? In this case, how do we submit an "no-op" pool rebalance root?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends what you mean by "execute a pool rebalance". It's possible in the current design to set up rules where the pool rebalance doesn't actually send tokens until the deficit gets pretty big, so there would be a rebalance leaf, but that leaf wouldn't result in tokens being sent.

);

// Before interacting with a particular chain's adapter, ensure that the adapter is set.
require(address(crossChainContracts[poolRebalanceLeaf.chainId].adapter) != address(0), "No adapter for chain");
Copy link
Member

Choose a reason for hiding this comment

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

is there any advantage to prevent setting the adapter contract address to 0x0 in the setCrossChainContracts func?

Copy link
Contributor Author

@mrice32 mrice32 Feb 26, 2022

Choose a reason for hiding this comment

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

I think that's a worthwhile check! Will add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think we should actually allow this to allow a chain to be de-whitelisted.

@mrice32 mrice32 requested a review from nicholaspai February 27, 2022 18:09

// l1 addresses are transformed during l1->l2 calls. See https://developer.offchainlabs.com/docs/l1_l2_messages#address-aliasing for more information.
// This cannot be pulled directly from Arbitrum contracts because their contracts are not 0.8.X compatible and this operation takes advantage of
// overflows, whose behavior changed in 0.8.0.
Copy link
Member

Choose a reason for hiding this comment

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

add a comment on the unchecked block below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Matt Rice <matthewcrice32@gmail.com>
@mrice32 mrice32 merged commit ee0eadb into master Feb 28, 2022
@mrice32 mrice32 deleted the remaining_todos branch February 28, 2022 02:52
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.

4 participants