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

Add dependent package objects to input_objects during module publish #300

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Jan 28, 2022

This PR fixes #284.
The problem here is that during module publishing, we will eventually need to read more objects on-chain due to module dependency (the to-be-published package can depend on other package objects). We need to first check if these objects exist on-chain before executing the publishing.
If we don't do so, in the case when some authorities are out-of-sync and don't contain some of the dependent packages, we could end up charging failure gas, leading to inconsistent state!
This PR fixes it by adding these implicit dependencies to input_objects so that they are checked the same way just like other input objects.
One specialty here is that we don't have the digest when retrieving these dependency objects, and we don't care about it anyway since published packages never change on-chain. To handle this, we add an InputType enum that gets returned along with each input object to indicate their types. Doing so allows us to skip the digest check on the dependency objects. It also makes it slightly cleaner to check on transfer object and gas object.

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.

Nice work!

fastpay_core/src/authority.rs Outdated Show resolved Hide resolved
fastpay_core/src/authority.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

This is a good catch, and a bug that needs fixing. But have a look at my suggestions. Basically we have a mechanism to track dependencies, and express dependencies which we should use uniformly.

fastpay_core/src/authority.rs Outdated Show resolved Hide resolved
fastpay_core/src/authority.rs Outdated Show resolved Hide resolved
@lxfind lxfind changed the title Check dependent packages exist before publishing Add dependent package objects to input_objects during module publish Jan 30, 2022
@lxfind
Copy link
Contributor Author

lxfind commented Jan 30, 2022

Redid the fix based on @gdanezis 's suggestion. Instead of checking it manually, these dependency packages are now part of the input_objects.

Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

Many thanks for integrating this into the input_objects logic. As a result now we also can sync the missing packages (hopefully, need to test) with no extra work.

fastpay_core/src/authority.rs Outdated Show resolved Hide resolved
fastx_types/src/messages.rs Outdated Show resolved Hide resolved
@lxfind
Copy link
Contributor Author

lxfind commented Jan 30, 2022

OK. Renamed the enum to InputObjectKind, as there are really only two kinds, move object or package.
For move object we put in object ref, for move package we only need object id.

Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

See the commend about checking packages are read-only object refs. Besides this I think this is good to go, but I fear some of the changes I landed will need some merging.

@@ -242,6 +244,28 @@ impl PartialEq for SignedOrder {
}
}

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum InputObjectKind {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that!

fastpay_core/src/authority.rs Outdated Show resolved Hide resolved
@lxfind lxfind merged commit 650daaf into main Jan 31, 2022
@lxfind lxfind deleted the check-dependent-packages branch January 31, 2022 17:58
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.

[move publishing] check that immediate dependencies of Move package exist, reject otherwise
3 participants