[native bridge smart contract 1/n] - move package #15411
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
patrickkuo
changed the title
patrickkuo
force-pushed
the
pat/native-bridge-move-code-only
branch
from
71d93ab to
5727e89
Compare
| version: u64, | ||
| chain_id: u8, | ||
| // nonce for replay protection | ||
| sequence_nums: VecMap<u8, u64>, |
There was a problem hiding this comment.
I'm thinking whether it's cost effective to just use variables to keep the sequence numbers, to save the cost of looking up in a map.
Is this over-optimization? @emmazzz
There was a problem hiding this comment.
I don't imagine the size of this vector to be over 10 ever so the cost should be pretty low anyway. Besides, this way is more extensible - keeping sequence numbers in fields means each time we add a new message type we'll need to upgrade BridgeInner type.
| version: u64, | ||
| chain_id: u8, | ||
| // nonce for replay protection | ||
| sequence_nums: VecMap<u8, u64>, |
There was a problem hiding this comment.
I don't imagine the size of this vector to be over 10 ever so the cost should be pretty low anyway. Besides, this way is more extensible - keeping sequence numbers in fields means each time we add a new message type we'll need to upgrade BridgeInner type.
| treasuries: ObjectBag | ||
| } | ||
|
|
||
| public fun create(ctx: &mut TxContext): BridgeTreasury { |
There was a problem hiding this comment.
Yes, and this is crucial if we ever want to change the signatures of functions here. It's always better to keep functions as private as possible.
|
|
||
| module bridge::message_types { | ||
| // message types | ||
| const TOKEN: u8 = 0; |
There was a problem hiding this comment.
Do we expect the number of message types to never be more than 255? If not may be worth changing this to a u16/u32.
There was a problem hiding this comment.
my jaw will drop if we cross that number lol
There was a problem hiding this comment.
unless we decide to support bridging of any arbitrary coins in the future, otherwise I don't see how we can cross this number.
There was a problem hiding this comment.
| struct BridgeMessageKey has copy, drop, store { | ||
| source_chain: u8, | ||
| message_type: u8, | ||
| bridge_seq_num: u64 |
There was a problem hiding this comment.
Just to make sure I'm following here a BridgeMessage is uniquely identified by the source chain, message type, and seqno. This means that all transactions from a given source chain are serialized, and bridging cannot occur between multiple chains in parallel correct?
There was a problem hiding this comment.
each chain will have it's own seq num, so bridging can occur in parallel from different chain, but on each chain bridging requests are processed in serial.
|
|
||
| public(friend) fun mint<T>(self: &mut BridgeTreasury, amount: u64, ctx: &mut TxContext): Coin<T> { | ||
| create_treasury_if_not_exist<T>(self, ctx); | ||
| let treasury = object_bag::borrow_mut(&mut self.treasuries, type_name::get<T>()); |
There was a problem hiding this comment.
You may want to consider an alternative to typename here (not sure if it's necessarily better, but it feels a bit nicer). In particular, you could use a type-indexed value to index the elements in the bag. So something like
struct TreasuryIndex<phantom T> has copy, drop, store { }
...
object_bag::borrow_mut(&mut self.treasuries, TreasuryIndex<T>{});and below in create_treasury_if_not_exist you could do something like
let type = TreasuryIndex<T>{};
...
if (type == TreasuryIndex<BTC>{}) {
...
} else if (type == TreasuryIndex<ETH>{}) {
...
} else if ...That or possibly using token_id to index it? Or is the typename used for better off-chain indexing?
There was a problem hiding this comment.
I tried that but looks like we cannot compare generic type...
69 │ let index = TreasuryIndex<T>{};
│ - Found: 'T'. It is not compatible with the other type.
70 │ if (index == TreasuryIndex<BTC>{}) {
│ ^^ --- Found: 'bridge::btc::BTC'. It is not compatible with the other type.
│ │
│ Incompatible arguments to '=='
## Description as titled ## Test Plan How did you test the new or updated feature? --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description Added BlockList, UpdateBridgeLimit and UpdateAssetPrice messages ## Test Plan How did you test the new or updated feature? --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
patrickkuo
force-pushed
the
pat/native-bridge-move-code-only
branch
from
24d317d to
79caf0c
Compare
There was a problem hiding this comment.
we need to write a ton of documentation about this. The lack of it today makes it pretty difficult to do reviews and follow the code.
We need all public functions and all structs to be well documented and clear.
One other critical action item: we have started this doc https://docs.google.com/document/d/1S5Y2VCXg6RfU4VVVSaZ287gvQIN7qjjxvx6B8WK1f4I/edit#heading=h.5g3o3dkpe61y about how to look at Move code. What to check. It's a work in progress but it is vital we look into this doc and make sure we are following and defining a check list for Move code.
| @@ -127,6 +130,15 @@ module sui::object { | |||
| } | |||
| } | |||
|
|
|||
| #[allow(unused_function)] | |||
There was a problem hiding this comment.
can you remind me again who is calling this? what is the process that brings you here?
There was a problem hiding this comment.
this is called by the validators during end of epoch transaction
| // Copyright (c) Mysten Labs, Inc. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| module bridge::message_types { |
There was a problem hiding this comment.
how likely are those values to change over time?
If I understand this, we can only change with framework upgrades, is that the plan moving forward?
| public fun token_id<T>(): u8 { | ||
| let coin_type = type_name::get<T>(); | ||
| if (coin_type == type_name::get<BTC>()) { | ||
| 1 |
There was a problem hiding this comment.
maybe we could define those in constants?
also those values seem very stable, wonder what is the best way to express that
There was a problem hiding this comment.
currently these are defined in a notion doc, we will add a readme.md for this once things are more stable
|
|
||
| // This function can only be called by the token recipient | ||
| // Returns None if the token has already been claimed. | ||
| public fun claim_token<T>(self: &mut Bridge, source_chain: u8, bridge_seq_num: u64, ctx: &mut TxContext): Option<Coin<T>> { |
There was a problem hiding this comment.
Should we consider returning Coin<T> and abort if it's already claimed? It will be hard to use this API otherwise.
There was a problem hiding this comment.
I see that we emit an event for already claimed tokens, but why do we want to charge gas for failed attempts and not just abort?
There was a problem hiding this comment.
the primary user of this method is the bridge client, this is designed this way to make it easy for the bridge client to process the messages without getting transaction errors, this was requested by @longbowlu
There was a problem hiding this comment.
I don't think that's possible for this function (but I might be missing context), it does indeed make sense on the one below (where we/anyone can trigger the claim for the recipient).
How would the bridge client sign on behalf of the recipient?
|
|
||
| // This function can be called by anyone to claim and transfer the token to the recipient | ||
| // If the token has already been claimed, it will return instead of aborting. | ||
| public fun claim_and_transfer_token<T>( |
There was a problem hiding this comment.
Curious on the motivation of this. What would be the case in which we'd want anyone to trigger the claim?
There was a problem hiding this comment.
this will be used by the bridge client, this function allow it to claim the token for the recipient.
|
|
||
| // Claim token from approved bridge message | ||
| // Returns Some(Coin) if coin can be claimed. If already claimed, return None | ||
| fun claim_token_internal<T>( |
There was a problem hiding this comment.
Do/Should we allow claims when bridge is frozen? (If not we need to assert)
There was a problem hiding this comment.
good catch, we should assert here
This reverts commit cc21afa.
…16253) This reverts commit cc21afa. ## Description Revert bridge changes until audit is done ## Test Plan How did you test the new or updated feature? --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description This PR contains all the move code for the new Sui native bridge (first draft) it require #15125 to pass all the test TODOs (in next PR): * Replace hardcoded committee with configuration * add coin burn limit per asset type * more test ## Test Plan How did you test the new or updated feature? --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes --------- Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com> (cherry picked from commit cc21afa)
## Description This PR contains all the move code for the new Sui native bridge (first draft) it require #15125 to pass all the test TODOs (in next PR): * Replace hardcoded committee with configuration * add coin burn limit per asset type * more test ## Test Plan How did you test the new or updated feature? --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes --------- Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com> (cherry picked from commit cc21afa)
Description
This PR contains all the move code for the new Sui native bridge (first draft)
it require #15125 to pass all the test
TODOs (in next PR):
Test Plan
How did you test the new or updated feature?
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes