sui icon indicating copy to clipboard operation
sui copied to clipboard

[native bridge smart contract 2/n] - Add Bridge package to Sui framework

Open patrickkuo opened this issue 2 years ago • 2 comments

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

patrickkuo avatar Nov 30 '23 13:11 patrickkuo

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

vercel[bot] avatar Nov 30 '23 13:11 vercel[bot]

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

patrickkuo avatar Dec 19 '23 16:12 patrickkuo

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

lxfind avatar Feb 09 '24 06:02 lxfind

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

patrickkuo avatar Feb 09 '24 15:02 patrickkuo

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?

longbowlu avatar Feb 09 '24 17:02 longbowlu

Looks good overall! left some questions about best practices. Would love @lxfind @mystenmark and @amnn 's review if possible

longbowlu avatar Feb 09 '24 23:02 longbowlu

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.

mystenmark avatar Feb 10 '24 00:02 mystenmark

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

mystenmark avatar Feb 10 '24 00:02 mystenmark

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

longbowlu avatar Feb 10 '24 01:02 longbowlu

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

mystenmark avatar Feb 10 '24 04:02 mystenmark

@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?

patrickkuo avatar Feb 13 '24 21:02 patrickkuo

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

patrickkuo avatar Feb 13 '24 21:02 patrickkuo

@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?

bmwill avatar Feb 13 '24 21:02 bmwill

@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.

longbowlu avatar Feb 13 '24 22:02 longbowlu

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

bmwill avatar Feb 13 '24 23:02 bmwill

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

longbowlu avatar Feb 13 '24 23:02 longbowlu

let's not get this in until people have time to digest it and let's talk about it

dariorussi avatar Feb 14 '24 00:02 dariorussi