forked from ethereum-optimism/optimism
-
Notifications
You must be signed in to change notification settings - Fork 8
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
Update token-bridging doc #156
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Maybe as a replacement we can add something in the form of - "To protect users, theses bridges only allow to bridge .... However, the light bridge offers an alt option to ..."
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 can add this back, though I don't really understand what it's documenting, since fundamentally, with only Boba Eth and Boba BNB, there is no combination of L1s where the bridging does not work. So I don't understand how we can say "To protect our users, we don't allow you to do this thing that doesn't actually exist".
I was trying to come up with a sentence to incorporate the light bridge, but there is no Eth Mainnet <-> BOBA BNB, nor Eth Mainnet <-> BNB Mainnet, as I read in the docs at least. So, I'm having trouble making it make sense in this context.
Any ideas?
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.
true - this needs some fixing. As i see it the line mostly wants to inform users that these bridges (EthBridge/AltL1Bridge) are setup (and meant to be) between Ethereum and BNB (L1) (because its the canonical way). Ethereum <> L2 should not be supported because of the finality on L2, and BNB <> Boba ETH wouldn't work either (because tokens have a source at Ethereum)
agree with the latter part about light-bridges
also not strictly a requirement unless you feel its informative to the user
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.
Given that this section is titled:
I feel like it's clear that this path is not intended for anything related to L2 bridging.
I do think this section could be improved, but my feeling is that removing is the clearest option for the moment.
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.
sure thing! makes sense