-
Notifications
You must be signed in to change notification settings - Fork 27
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
Teleporter registry upgrade #43
Conversation
contracts/src/CrossChainApplications/ExampleMessenger/ExampleCrossChainMessenger.sol
Fixed
Show fixed
Hide fixed
contracts/src/CrossChainApplications/VerifiedBlockHash/BlockHashPublisher.sol
Fixed
Show fixed
Hide fixed
contracts/src/CrossChainApplications/VerifiedBlockHash/BlockHashReceiver.sol
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for creating the followup issues. Excited to get this merged 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
a2d0d44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
000a894
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
}) | ||
); | ||
messageID = teleporterRegistry | ||
.getLatestTeleporter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to confirm that we never plan to send out blockhash using an older version of teleporter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no don't think so, any particular use case you're thinking of?
Signed-off-by: minghinmatthewlam <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
bridge_a_deploy_result=$(forge create --private-key $user_private_key --constructor-args $teleporter_contract_address \ | ||
--rpc-url $subnet_a_url src/CrossChainApplications/ERC20Bridge/ERC20Bridge.sol:ERC20Bridge) | ||
# Deploy the ERC20 bridge contract to all chains. | ||
bridge_a_deploy_result=$(forge create --private-key $user_private_key \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a need to move --constructor-args
to be the last argument here with the last foundry version, or did you just move it to be consistent in our usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not due to last version, but on their page they say "The --constructor-args flag must be positioned last in the command, since it takes multiple values.", even though it seemed to work previously without being last position. https://book.getfoundry.sh/reference/forge/forge-create
Why this should be merged
Provides upgradeability of Teleporter through a registry contract #7. Creates a warp protocol registry that can be inherited by warp protocols, and for Teleporter specifically has
TeleporterRegistry
.How this works
Adds a warp registry contract that gets inherited by
TeleporterRegistry
. The registry can add new protocol versions through a warp out of band message, then keeps track of the version and protocol address mappings. The constructor of the registry also allows for initialization, so that the registry initially doesn't need an additional warp out of band message if Teleporter is already deployed.Cross chain apps will take in the registry instead of previous passing Teleporter in their constructor. The best practice for sending will be to ask the registry every time for sending with the latest version. For receiving, cross chain apps are recommended to have a
minTeleporterVersion
, and for theirITeleporterReceiver.receiveTeleporterMessage
implementation to check that themsg.sender
matches aversion > _minTeleporterVersion
.TeleporterUpgradeable
provides these best practices as an abstract contract that cross chain apps built on Teleporter can inherit and use.How this was tested
Unit Tests, adding tests for Teleporter/Warp registry and
TeleporterUpgradeable
E2E test update to pass in registry for cross chain apps while warp out of band messages are still being implemented.
How is this documented
upgrade README