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

[client/publish] Add ability to publish --with-unpublished-dependencies #7426

Merged
merged 4 commits into from
Jan 20, 2023

Conversation

amnn
Copy link
Member

@amnn amnn commented Jan 16, 2023

If a package depends on other packages that haven't been published yet, this flag offers a convenience to publish them as part of the same transaction (or to include them in the base64 dump when building).

This is not ideal from the perspective of code re-use, but does help while wipes are more common.

This also has implications for source validation, which needs to support validating packages that have other packages embedded in them:

  • Now, on-chain modules in the root package could be matched against multiple source packages, so they are fetched all in one go.
  • This also means there is no longer a 1:1 correspondence between numerical address and on-chain address, so the LocalDependencyNotFound error now identifies a package by its address.

Test Plan

New source-validation and move integration tests:

$ cargo simtest
$ cargo nextest run

@amnn amnn added the devx label Jan 16, 2023
@amnn amnn self-assigned this Jan 16, 2023
@vercel
Copy link

vercel bot commented Jan 16, 2023

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

4 Ignored Deployments
Name Status Preview Comments Updated
explorer ⬜️ Ignored (Inspect) Jan 20, 2023 at 3:19PM (UTC)
explorer-storybook ⬜️ Ignored (Inspect) Jan 20, 2023 at 3:19PM (UTC)
frenemies ⬜️ Ignored (Inspect) Jan 20, 2023 at 3:19PM (UTC)
wallet-adapter ⬜️ Ignored (Inspect) Jan 20, 2023 at 3:19PM (UTC)

// SAFETY: package built successfully
let modules = graph.compute_topological_order().unwrap();

if with_unpublished_deps {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the heart of the change -- including all modules whose self address is 0x0, instead of just the modules in the root package.

let mut map = OnChainBytes::new();

for addr in addresses {
let SuiRawMovePackage { module_map, .. } = self.pkg_for_address(&addr).await?;
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to be able to make these requests in parallel, but the fact that tokio::spawn requires a 'static lifetime bound which self will not outlive was a source of issues.

Any tips appreciated!

Copy link
Contributor

Choose a reason for hiding this comment

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

Summoning @bmwill

Copy link
Contributor

Choose a reason for hiding this comment

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

I just pushed a commit on top of this PR's branch that changes these to be done concurrently (instead of 1 after another). Feel free to take it, change it, or remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Love it, thank you! Also, TIL that you can push to other people's forks. @bmwill, operationally, how does that work -- are you just setting my fork as an upstream and pushing to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to tweak the use of futures to just join_all in the end because the streams API was inducing a higher-kinded lifetime error in another part of the codebase, strangely, but join_all works! Thanks for the pointers.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a setting you can toggle to "allow maintainers to modify" that only gives permissions to maintainers of an upstream repo to push to the branch you use for your PR on your own personal fork

crates/sui/src/client_commands.rs Show resolved Hide resolved
let mut map = OnChainBytes::new();

for addr in addresses {
let SuiRawMovePackage { module_map, .. } = self.pkg_for_address(&addr).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Summoning @bmwill

amnn and others added 4 commits January 20, 2023 15:18
If a package depends on other packages that haven't been published
yet, this flag offers a convenience to publish them as part of the
same transaction (or to include them in the base64 dump when
building).

This is not ideal from the perspective of code re-use, but does help
while wipes are more common.

This also has implications for source validation, which needs to
support validating packages that have other packages embedded in them:

- Now, on-chain modules in the root package could be matched against
  multiple source packages, so they are fetched all in one go.
- This also means there is no longer a 1:1 correspondence between
  numerical address and on-chain address, so the
  `LocalDependencyNotFound` error now identifies a package by its address.

Test Plan:

New source-validation and move integration tests:

```
$ cargo simtest
$ cargo nextest run
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants