Disallow the same bridge on one chain: #4720
Merged
Conversation
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
mvadari
reviewed
Sep 21, 2023
src/test/app/XChain_test.cpp
Outdated
| @@ -631,10 +632,12 @@ struct XChain_test : public beast::unit_test::suite, | |||
| env.tx(create_bridge(a1, bridge(a1, GUSD, B, BUSD))).close(); | |||
| env.tx(create_bridge(a2, bridge(a2, GUSD, B, BUSD))).close(); | |||
|
|
|||
| // Add the exact same bridge to two different accoutns (one must locking | |||
| // account and one must be issuing) | |||
| // Add the exact same bridge to two different accoutns (one locking | |||
There was a problem hiding this comment.
While you're here:
Suggested change
| // Add the exact same bridge to two different accoutns (one locking | |
| // Add the exact same bridge to two different accounts (one locking |
There was a problem hiding this comment.
Fixed in 211ccc6fd9 [fold] Add additional test
mvadari
reviewed
Sep 21, 2023
Comment on lines
637
to
+642
| env.tx(create_bridge(a3, bridge(a3, GUSD, a4, a4["USD"]))).close(); | ||
| env.tx(create_bridge(a4, bridge(a3, GUSD, a4, a4["USD"]))).close(); | ||
| env.tx(create_bridge(a4, bridge(a3, GUSD, a4, a4["USD"])), | ||
| ter(tecDUPLICATE)) | ||
| .close(); |
There was a problem hiding this comment.
Perhaps this should check the reverse situation compared to the above test change - issuing chain then locking chain.
There was a problem hiding this comment.
Fixed in 211ccc6fd9 [fold] Add additional test
mvadari
approved these changes
Sep 26, 2023
scottschurr
approved these changes
Sep 27, 2023
| { | ||
| return tecDUPLICATE; | ||
| auto hasBridge = [&](STXChainBridge::ChainType ct) -> bool { | ||
| return !!ctx.view.read(keylet::bridge(bridgeSpec, ct)); |
There was a problem hiding this comment.
As a small optimization, consider calling ctx.view.exists() instead, like this:
return ctx.view.exists(keylet::bridge(bridgeSpec, ct));
This will avoid deserializing the bridge to check the LedgerEntryType. Just a thought.
There was a problem hiding this comment.
Love it. Fixed in 8bbf7b9e21 [fold] Use exists instead of read to check if ledger object exists
Before this patch, two door accounts on the same chain could could own the same bridge spec (of course, one would have to be the issuer and one would have to be the locker). While this is silly, it does not violate any bridge invariants. However, on further review if we allow this then the `claim` transactions would need to change. Since it's hard to see a use case for two doors to own the same bridge, this patch disallows it. (The transaction will return tecDUPLICATE).
seelabs
force-pushed
the
xbridge-disallow-same-bridge
branch
from
8bbf7b9 to
df63e1f
Compare
|
Squashed and rebased on the latest dev. This is ready to merge into develop. |
seelabs
added
the
Passed
|
note: have tests for this (Ram) |
sophiax851
pushed a commit
to sophiax851/rippled
that referenced
this pull request
Jun 12, 2024
Modify the `XChainBridge` amendment. Before this patch, two door accounts on the same chain could could own the same bridge spec (of course, one would have to be the issuer and one would have to be the locker). While this is silly, it does not violate any bridge invariants. However, on further review, if we allow this then the `claim` transactions would need to change. Since it's hard to see a use case for two doors to own the same bridge, this patch disallows it. (The transaction will return tecDUPLICATE).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Before this patch, two door accounts on the same chain could could own the same bridge spec (of course, one would have to be the issuer and one would have to be the locker). While this is silly, it does not violate any bridge invariants. However, on further review if we allow this then the
claimtransactions would need to change. Since it's hard to see a use case for two doors to own the same bridge, this patch disallows it. (The transaction will return tecDUPLICATE).