Skip to content
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

Merged
merged 5 commits into from
Mar 22, 2023

Conversation

emmazzz
Copy link
Contributor

@emmazzz emmazzz commented Mar 21, 2023

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)

  • 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

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.

@vercel
Copy link

vercel bot commented Mar 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
sui-wallet-kit ❌ Failed (Inspect) Mar 22, 2023 at 2:14AM (UTC)
3 Ignored Deployments
Name Status Preview Comments Updated
explorer ⬜️ Ignored (Inspect) Visit Preview Mar 22, 2023 at 2:14AM (UTC)
explorer-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 22, 2023 at 2:14AM (UTC)
wallet-adapter ⬜️ Ignored (Inspect) Visit Preview Mar 22, 2023 at 2:14AM (UTC)

@github-actions github-actions bot added the Type: Documentation Improvements or additions to documentation label Mar 21, 2023
@emmazzz emmazzz force-pushed the separate_framework branch from a600257 to f3f1386 Compare March 21, 2023 02:15
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 21, 2023 02:16 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 21, 2023 02:17 Inactive
@emmazzz emmazzz force-pushed the separate_framework branch from f3f1386 to cb13fce Compare March 21, 2023 02:37
@vercel vercel bot temporarily deployed to Preview – wallet-adapter March 21, 2023 02:38 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 21, 2023 02:38 Inactive
@emmazzz emmazzz marked this pull request as ready for review March 21, 2023 02:42
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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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

@emmazzz emmazzz force-pushed the separate_framework branch 2 times, most recently from 5d760c4 to 47fd066 Compare March 21, 2023 06:35
Copy link
Contributor

@bmwill bmwill left a 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.

Comment on lines +942 to +948
(
SuiSystem::as_modules(),
SuiSystem::transitive_dependencies(),
),
Copy link
Contributor

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.

Copy link
Contributor Author

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" }
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫠

Copy link
Member

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!

@emmazzz emmazzz force-pushed the separate_framework branch from 47fd066 to 554b2ef Compare March 21, 2023 19:57
@emmazzz emmazzz requested a review from joyqvq as a code owner March 21, 2023 19:57
Copy link
Contributor

@lxfind lxfind left a 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

Copy link
Member

@amnn amnn left a 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injection LGTM!

crates/sui-core/src/unit_tests/authority_tests.rs Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@ version = "0.0.0"

[dependencies]
Examples = { local = "../object_basics" }
Sui = { local = "../../../../../sui-framework" }
Sui = { local = "../../../../../sui-framework/packages/sui-framework" }
Copy link
Member

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!

crates/sui-framework-build/src/compiled_package.rs Outdated Show resolved Hide resolved
crates/sui-framework-build/src/compiled_package.rs Outdated Show resolved Hide resolved
crates/sui-framework/docs/linked_table.md Show resolved Hide resolved
crates/sui-framework/packages/sui-system/Move.toml Outdated Show resolved Hide resolved
crates/sui-types/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

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!

crates/sui/tests/protocol_version_tests.rs Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 21, 2023 20:00 Inactive
@vercel vercel bot temporarily deployed to Preview – sui-wallet-kit March 21, 2023 22:20 Inactive
@emmazzz
Copy link
Contributor Author

emmazzz commented Mar 21, 2023

Addressed your comments and @bmwill's in the latest commit @amnn .

@emmazzz emmazzz requested a review from patrickkuo March 21, 2023 22:38
Copy link
Contributor

@patrickkuo patrickkuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rosetta, SDK LGTM

@emmazzz emmazzz requested review from amnn, bmwill and tnowacki March 21, 2023 23:00
Copy link
Contributor

@tzakian tzakian left a 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!

@emmazzz emmazzz force-pushed the separate_framework branch from e70f90b to b0be221 Compare March 21, 2023 23:14
@vercel vercel bot temporarily deployed to Preview – sui-wallet-kit March 21, 2023 23:14 Inactive
@emmazzz emmazzz force-pushed the separate_framework branch from b0be221 to ec4b6b6 Compare March 21, 2023 23:15
@vercel vercel bot temporarily deployed to Preview – sui-wallet-kit March 21, 2023 23:16 Inactive
@emmazzz emmazzz force-pushed the separate_framework branch from ec4b6b6 to f87cf45 Compare March 22, 2023 00:37
@emmazzz emmazzz force-pushed the separate_framework branch from f87cf45 to ba1f876 Compare March 22, 2023 02:14
@emmazzz emmazzz requested a review from oxade as a code owner March 22, 2023 02:14
@emmazzz emmazzz enabled auto-merge (squash) March 22, 2023 02:54
@emmazzz emmazzz merged commit da72e73 into MystenLabs:main Mar 22, 2023
benr-ml pushed a commit that referenced this pull request Mar 22, 2023
## 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.
@gegaowp gegaowp mentioned this pull request Mar 23, 2023
gegaowp added a commit that referenced this pull request Mar 23, 2023
## 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
danaiballa pushed a commit that referenced this pull request Mar 23, 2023
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants