-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
[sui-framework] separate sui-framework into two packages #9618
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
a600257
to
f3f1386
Compare
f3f1386
to
cb13fce
Compare
if module_id != &ModuleId::new(SUI_FRAMEWORK_ADDRESS, SUI_SYSTEM_MODULE_NAME.to_owned()) | ||
&& is_mutable_clock(view, param) | ||
{ | ||
if is_mutable_clock(view, param) { |
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.
What happened to this case?
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.
That function was moved out of sui system module and made private so not an entry fun anymore
5d760c4
to
47fd066
Compare
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.
This generally looks good. left a comment on one thing that needs to be fixed for genesis.
( | ||
SuiSystem::as_modules(), | ||
SuiSystem::transitive_dependencies(), | ||
), |
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.
You'll need to add this as a parameter in the call to create_genesis_context
above as well on like 923.
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.
Addressed this in the latest commit
@@ -4,7 +4,7 @@ version = "0.0.0" | |||
|
|||
[dependencies] | |||
Examples = { local = "../object_basics" } | |||
Sui = { local = "../../../../../sui-framework" } | |||
Sui = { local = "../../../../../sui-framework/packages/sui-framework" } |
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.
how many ../'s can we have lol
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.
🫠
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.
Currently we top at 6!
47fd066
to
554b2ef
Compare
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.
LGTM
May want to get a final stamp from Move team before merge
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.
Changes to system package injection LGTM, holding off on the stamp for folks to review TS stuff (+ pending comments from @bmwill) but all good from my side modulo some naming nits!
@@ -3523,7 +3535,7 @@ mod tests { | |||
} | |||
|
|||
#[cfg(msim)] | |||
pub mod sui_framework_injection { | |||
pub mod sui_system_injection { |
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.
Injection LGTM!
@@ -4,7 +4,7 @@ version = "0.0.0" | |||
|
|||
[dependencies] | |||
Examples = { local = "../object_basics" } | |||
Sui = { local = "../../../../../sui-framework" } | |||
Sui = { local = "../../../../../sui-framework/packages/sui-framework" } |
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.
Currently we top at 6!
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.
Changes here seem right to me as well!
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.
rosetta, SDK LGTM
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.
Changes look good to me!
e70f90b
to
b0be221
Compare
b0be221
to
ec4b6b6
Compare
ec4b6b6
to
f87cf45
Compare
f87cf45
to
ba1f876
Compare
## Description This PR splits our `sui-framework` Move code into two packages, one for modules in `governance` folder and one for the rest of the modules. Most of the changes here are pretty mechanical, mostly about making sure we use the right package(s) and correct path. ## Test Plan Existing tests should pass. --- 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) - [x] user-visible impact - [x] breaking change for a client SDKs - [x] breaking change for FNs (FN binary must upgrade) - [x] breaking change for validators or node operators (must upgrade binaries) - [x] breaking change for on-chain data layout - [x] necessitate either a data wipe or data migration ### Release notes `sui-framework` Move package is split up into two packages, `sui-framework` and `sui-system`. `sui-system` contains modules that were in the `governance` folder.
## Description Fixed CI, it was b/c the splitting that happened in #9618 A couple of other changes: - added checkpoint read test around epoch boundary - add timeout to wait_until ## Test Plan integration test both locally and on CI
## Description Fixed CI, it was b/c the splitting that happened in #9618 A couple of other changes: - added checkpoint read test around epoch boundary - add timeout to wait_until ## Test Plan integration test both locally and on CI
Description
This PR splits our
sui-framework
Move code into two packages, one for modules ingovernance
folder and one for the rest of the modules. Most of the changes here are pretty mechanical, mostly about making sure we use the right package(s) and correct path.Test Plan
Existing tests should pass.
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
sui-framework
Move package is split up into two packages,sui-framework
andsui-system
.sui-system
contains modules that were in thegovernance
folder.