sui
sui copied to clipboard
[native bridge smart contract 2/n] - Add Bridge package to Sui framework
Description
This PR add bridge package to the Sui framework
also made some changes to :
- make bridge object exempt for id leaks checks
- make bridged currencies exempt for OTW checks
- added bridge creation to end of epoch transaction
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| mysten-ui | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Feb 16, 2024 2:17pm |
| sui-core | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Feb 16, 2024 2:17pm |
| sui-typescript-docs | ✅ Ready (Inspect) | Visit Preview | 💬 Add feedback | Feb 16, 2024 2:17pm |
3 Ignored Deployments
| Name | Status | Preview | Comments | Updated (UTC) |
|---|---|---|---|---|
| explorer | ⬜️ Ignored (Inspect) | Visit Preview | Feb 16, 2024 2:17pm | |
| multisig-toolkit | ⬜️ Ignored (Inspect) | Visit Preview | Feb 16, 2024 2:17pm | |
| sui-kiosk | ⬜️ Ignored (Inspect) | Visit Preview | Feb 16, 2024 2:17pm |
this PR is ready for second round review, I have moved all the move framework changes to https://github.com/MystenLabs/sui/pull/15411 to make this PR smaller
Is this PR trying to both add a new package and creating new bridge objects? Is there a reason they have to be in the same PR? It would make it easier to review if they are in separate ones if possible
Is this PR trying to both add a new package and creating new bridge objects? Is there a reason they have to be in the same PR? It would make it easier to review if they are in separate ones if possible
I have moved all new package code to https://github.com/MystenLabs/sui/pull/15411, this PR now only contain creating new bridge package.
I will do more clean up to make this easier to review
Is this PR trying to both add a new package and creating new bridge objects? Is there a reason they have to be in the same PR? It would make it easier to review if they are in separate ones if possible
I have moved all new package code to #15411, this PR now only contain creating new bridge package.
I will do more clean up to make this easier to review
but this makes https://github.com/MystenLabs/sui/pull/15411 hard to review (i've read all other PRs but not the new package code closely yet)
is "new package code" just https://github.com/MystenLabs/sui/pull/15411/commits/dc5b853afb399076efd3e3569b44870c45662635?
Looks good overall! left some questions about best practices. Would love @lxfind @mystenmark and @amnn 's review if possible
Overall the bridge object creation logic looks ok. Main thing I want to understand is what the chain id is, and where it comes from.
Oh, also, this needs a test for the bridge object creation. See https://github.com/MystenLabs/sui/blob/04bfc611208ac78b13f8eced6382ebb646d1be74/crates/sui-e2e-tests/tests/zklogin_tests.rs#L154 for an example
Overall the bridge object creation logic looks ok. Main thing I want to understand is what the chain id is, and where it comes from.
this is chain id for bridge
Overall the bridge object creation logic looks ok. Main thing I want to understand is what the chain id is, and where it comes from.
this is chain id for bridge
can we call it "bridge chain id" or something? we already have a ChainIdentifier https://github.com/MystenLabs/sui/blob/67e52ff3e9256ba9c648ef2ae329f57e1f99a599/crates/sui-types/src/digests.rs#L158
@lxfind @amnn @mystenmark , I am setting the bridge_chain_id to devnet for now, and will have a subsequent PR to set this dynamically when bridge is ready for testnet. Do you guys think this is ok?
Still want to take some more time. Just want to quickly note that we can definitely just pass in the UID directly. Not sure what else that simplifies anything
Didn’t know this is possible! Will make the change
@tnowacki whats the current state of how we rollout framework changes to mainnet, as in if we land this will this go out to mainnet in 2 weeks? If so, do we actually want this to go out to mainnet in two weeks or do we want to hold off until we're ready to launch this?
@tnowacki whats the current state of how we rollout framework changes to mainnet, as in if we land this will this go out to mainnet in 2 weeks? If so, do we actually want this to go out to mainnet in two weeks or do we want to hold off until we're ready to launch this?
If this is cut in 1.19 (this friday), it will go to testnet next week and mainnet the week after. It means the move package and related rust code will be there, but object creation won't happen until we turn on the bridge flag in protocol config. In this PR, the flag is only turned on in devnet.
I sort of think that's a problem, unless were actually ok with this rolling out to testnet and mainnet. We shouldn't be releasing any new framework code unless we're happy with it and ready to move forward. Maybe we are, if so you can ignore my concerns
I sort of think that's a problem, unless were actually ok with this rolling out to testnet and mainnet. We shouldn't be releasing any new framework code unless we're happy with it and ready to move forward. Maybe we are, if so you can ignore my concerns
We have to test this on testnet for an indefinite amount of time (e.g. 4 weeks). It would be fairly hard, if possible at all to hold this from mainnet given the scope of this change. The part that wires up the package is straightforward enough, so as long as these code is correct, we can update the move code later on relatively easy and not intrusively. The most important thing is to make sure no related objects are gonna be created in testnet/mainnet until we want them to. If there is an easy way to isolate the framework code per network, I'll indeed feel more comfortable doing that. Unfortunately this does not exist afaik. But perhaps it should? @dariorussi
let's not get this in until people have time to digest it and let's talk about it