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

[framework] Adds publisher module #7196

Merged
merged 6 commits into from Jan 12, 2023
Merged

[framework] Adds publisher module #7196

merged 6 commits into from Jan 12, 2023

Conversation

damirka
Copy link
Contributor

@damirka damirka commented Jan 6, 2023

Finalizes the experiment myself and @amnn started; and proposes a publisher module.

The goal of this module is to enable recognition of the Publisher authority. Potentially it could be used for:

  1. Setting rules / registering a type in a system, such as metadata, AFTER the module was published
  2. Authorizing admin actions (effectively replacing AdminCap which is often added manually)

@vercel
Copy link

vercel bot commented Jan 6, 2023

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

2 Ignored Deployments
Name Status Preview Comments Updated
explorer-storybook ⬜️ Ignored (Inspect) Jan 11, 2023 at 10:54PM (UTC)
wallet-adapter ⬜️ Ignored (Inspect) Jan 11, 2023 at 10:54PM (UTC)

Copy link
Contributor

@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.

I like it! Just waiting to finish the conversation about a home for things that should eventually belong in move stdlib before final stamp.

crates/sui-framework/sources/publisher.move Outdated Show resolved Hide resolved
crates/sui-framework/sources/publisher.move Outdated Show resolved Hide resolved
crates/sui-framework/sources/publisher.move Outdated Show resolved Hide resolved
crates/sui-framework/sources/publisher.move Show resolved Hide resolved
@damirka
Copy link
Contributor Author

damirka commented Jan 9, 2023

@amnn @sblackshear on the topic: do you think we should rename it into PublisherCap instead of Publisher?

@amnn
Copy link
Contributor

amnn commented Jan 9, 2023

do you think we should rename it into PublisherCap instead of Publisher?

I think technically it's a witness (i.e. it witnessed the package being published), rather than a capability.

@damirka
Copy link
Contributor Author

damirka commented Jan 11, 2023

@amnn @sblackshear

I've addressed your comments above. Few changes:

  • adds std::address library with a length() function
  • adds address getter + module name getter in std::type_name
  • does a little cleanup in our sui::bcs library to switch to sui::address instead of using const for ADDRESS length

Copy link
Contributor

@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.

The Publisher changes look good to me, but unless I'm missing something (very possible), I think type_name::get_address and type_name::get_module produce reversed outputs compared to what I would expect (and could benefit from some tests) -- could you take a look at those?

let str_bytes = ascii::as_bytes(&self.name);
let addr_bytes = vector[];

// Reverse-read string from the last byte pushing the value
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we are reverse reading the address here? IIUC, this means addr_bytes will be the reverse of the address prefix of str_bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yrros esrever ni gnikniht saw daeh ym

crates/sui-framework/sources/publisher.move Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Thanks @damirka !

crates/sui-framework/sources/publisher.move Outdated Show resolved Hide resolved
@@ -103,6 +111,10 @@ module sui::test_publisher {
assert!(publisher::is_package<CustomType>(&pub), 0);
assert!(publisher::is_package<Scenario>(&pub), 0);

assert!(&ascii::string(b"0000000000000000000000000000000000000002") == publisher::package(&pub), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: These tests probably belong in the stdlib rather than here, as that's where most of the logic is -- but very happy to see them!

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's a tricky part. We don't have tests in the stdlib and I never dared to add one. Guess it's a bigger question on whether it's okay to have tests for stdlib or there's a better place for them where they live?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, interesting -- maybe @sblackshear / @tnowacki has context on why there seems to not be #[test]s in the stdlib?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Any case, not a blocker for this PR)

@damirka damirka merged commit 091949f into MystenLabs:main Jan 12, 2023
@damirka damirka deleted the ds/publisher-cap branch January 12, 2023 11:26
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.

None yet

3 participants