Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SC-452] Refactor forwarders and receivers #17

Merged
merged 50 commits into from
Jun 12, 2024

Conversation

hexonaut
Copy link
Contributor

@hexonaut hexonaut commented Jun 1, 2024

This PR does two refactors:

  1. Split out XChainForwarders into separate libraries for better organization. I've also added some missing L2 -> L1 messages on each forwarder.
  2. Receivers don't need to be extended, as the logic will always proxy a function call such as here: https://github.com/marsfoundation/spark-gov-relay/blob/master/src/receivers/BridgeExecutorReceiverArbitrum.sol#L19 . This update makes receivers generic message-passers.

You can see concrete example of how these changes improve usage here: https://github.com/marsfoundation/spark-gov-relay/pull/18/files and here: https://github.com/marsfoundation/xchain-dsr-oracle/pull/20/files

@hexonaut hexonaut marked this pull request as ready for review June 4, 2024 13:23
Base automatically changed from SC-440-refactor-e2e to master June 7, 2024 09:03
src/receivers/ArbitrumReceiver.sol Show resolved Hide resolved
src/receivers/OptimismReceiver.sol Show resolved Hide resolved
src/receivers/OptimismReceiver.sol Show resolved Hide resolved
src/receivers/AMBReceiver.sol Show resolved Hide resolved
src/receivers/AMBReceiver.sol Show resolved Hide resolved
README.md Outdated
@@ -1,5 +1,20 @@
# xchain-helpers

This repository contains tooling for multichain testing such as cross-chain e2e testing support.
This repository three tools for use with multi-chain development. Domains refer to blockchains which are connected by bridges. Domains may have multiple bridges connecting them, for example both the Optimism Native Bridge and Circle CCTP connect Ethereum and Optimism domains.
Copy link
Contributor

Choose a reason for hiding this comment

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

has three

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


## Receivers

The most common pattern is to have an authorized contract forward a message to another "business logic" contract to abstract away bridge dependencies. Receivers are contracts which perform this generic translation - decoding the bridge-specific message and forwarding to another `target` contract. The `target` contract should have logic to restrict who can call it and permission this to one or more bridge receivers.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should talk about how the receiver itself has the restrictions/requires specific to the bridge and talk about the use of fallbacks and how it routs the message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added another paragraph


// Use Optimism for failure tests as the code logic is the same

function test_invalidSourceAuthority() public {
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be third

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


function test_base() public {
checkOptimismStyle(getChain("base").createFork());
function test_invalidSourceAuthority() public {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be second

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

function test_optimism() public {
checkOptimismStyle(getChain("optimism").createFork());
}
// Use Arbitrum One for failure test as the code logic is the same
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

lucas-manuel
lucas-manuel previously approved these changes Jun 11, 2024
@hexonaut hexonaut merged commit e1f9443 into master Jun 12, 2024
3 checks passed
@hexonaut hexonaut deleted the SC-452-refactor-forwards-receivers branch June 12, 2024 16:17
Copy link

Coverage after merging SC-452-refactor-forwards-receivers into master will be

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/forwarders
   AMBForwarder.sol100%100%100%100%
   ArbitrumForwarder.sol100%100%100%100%
   CCTPForwarder.sol100%100%100%100%
   OptimismForwarder.sol100%100%100%100%
src/receivers
   AMBReceiver.sol100%100%100%100%
   ArbitrumReceiver.sol100%100%100%100%
   CCTPReceiver.sol100%100%100%100%
   OptimismReceiver.sol100%100%100%100%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants