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

Publish one package at a time #185

Merged
merged 1 commit into from
Jan 15, 2022
Merged

Publish one package at a time #185

merged 1 commit into from
Jan 15, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Jan 15, 2022

In the existing implementation, when publishing a list of modules, we group all the modules by their address, and generate one package per address. This means one can publish multiple packages at a time. This has several disadvantages:

  1. The implementation is much more complicated.
  2. Developers might mistakenly publish more than necessary (e.g. published dependencies by accident).
  3. At the end of the day, the initial address that developers write on Move.toml doesn't matter, so we might as well enforce it to be 0x0 to avoid confusions. To do so, we could not publish more than one packages at a time because they would all have same address.

This PR changes it to publish one package at a time. I also had some interesting realizations that allow me to further simply the implementation.
A summary of this PR:

  1. Changed generate_package_info_map to generate_package_id, since we no longer generate multiple packages.
  2. In the genesis, instead of collecting modules from both fastx and stdlib, we now generate a package for each of fastx and stdlib separately.
  3. The fun part: After compiling a package, we usually generate a new object ID, and then rewrite all the module IDs with this new ID. This is necessary for user-published packages because we cannot let developers arbitrarily define object IDs. However, we don't have to do this for fastx framework and move stdlib package! They can remain as 0x1 and 0x2 without any issues as long as we don't rewrite the module ids. This allows us to completely get rid of the last hardcoded long hex addresses.
  4. The examples directory is moved out of framework to avoid being published together into genesis.
  5. Fixed a bug in move unit tests: we are not checking the test result properly.
  6. In authority_tests, we are not checking the status of the result of handle confirmation order. Fix it.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

The implementation is really neat, thanks!

The question for me is more about the UX of deploying interdependent smart contracts in a world where the sequencing of publishing operations is not up to the user. With this PR, semantic publication constraints of interdependent modules needs to be pushed "down" to the semantics of several transactions, as we know how to specify that a transaction has others as pre-requisites.

That's fine, and makes sense, but I'm wondering if we could have a look at user-facing error messages triggered upon simultaneous publication of several modules, and try to have them drive the user towards the right approach.

Comment on lines 230 to 238
.await?;
assert!(result
.signed_effects
.as_ref()
.unwrap()
.effects
.status
.is_ok());
Ok(result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: in tests, panics are actually a tad more useful than Results because of their stack traces. YMMV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me roll back this change and address it separately. I accidentally changed the semantics of this function. (it's supposed to return Result so caller can check when it's expected to fail)

@sblackshear
Copy link
Collaborator

That's fine, and makes sense, but I'm wondering if we could have a look at user-facing error messages triggered upon simultaneous publication of several modules, and try to have them drive the user towards the right approach.

Totally agreed that we need to be as helpful as possible here. The Move CLI has some features that I think we can steal/mimic/enhance in the FastX client:

  • When the client prepares a "publish package" transaction, it should locally link the modules in its package with the on-chain ones before sending the transaction. It will be a lot easier to explain what went wrong in a setting where we can afford to spend some time finding a detailed/helpful explanation (since the bytecode verifier and linker don't try very hard + probably shouldn't). The move sandbox doctor command is a useful example to mimic
  • You could imagine requiring that the client have certificates for all dependent packages before publishing. This ensures that any authority the client talks to will eventually know about all dependencies. The client could also query authorities to check for the presence of the package object and/or these certificates.

With this PR, semantic publication constraints of interdependent modules needs to be pushed "down" to the semantics of several transactions, as we know how to specify that a transaction has others as pre-requisites.

This becomes a problem if user A publishes a package and then user B immediately publishes one that depends on it before all authorities (or to be precise, the ones B is talking to) learn about A's package. But hopefully this interval will be very short. I'm hoping transactions within this interval won't very common in practice (i.e., the more typical flow will be that A publishes, announces their launch everywhere, B sees it and decides to build on it, then publishes days/weeks/months later), but definitely a case that we should handle gracefully regardless.

Copy link
Collaborator

@sblackshear sblackshear left a comment

Choose a reason for hiding this comment

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

Fantastic simplifications!

@lxfind
Copy link
Contributor Author

lxfind commented Jan 15, 2022

This becomes a problem if user A publishes a package and then user B immediately publishes one that depends on it before all authorities (or to be precise, the ones B is talking to) learn about A's package. But hopefully this interval will be very short. I'm hoping transactions within this interval won't very common in practice (i.e., the more typical flow will be that A publishes, announces their launch everywhere, B sees it and decides to build on it, then publishes days/weeks/months later), but definitely a case that we should handle gracefully regardless.

This is actually easy to avoid. One check I probably should add, is that during publishing, all dependencies must not have the same address as the ones being published. As long as we can enforce this, we don't need to worry about the timing, because one needs to successfully publish a package, obtain the address, update the Move.toml before it can even become a dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants