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

Update resource_groups.md #35

Merged
merged 3 commits into from
Jan 16, 2023
Merged

Update resource_groups.md #35

merged 3 commits into from
Jan 16, 2023

Conversation

davidiw
Copy link
Contributor

@davidiw davidiw commented Jan 15, 2023

Adding some updates after wrapping up the first complete implementation:

  • Took in Alden's suggestion on renaming the attribute fields
  • Cleaned up some wonky text that had somehow gotten in
  • Tightened up some of the writing for better readability

AIP seems like it is probably ready for community review. Will discuss with gatekeepers.

@@ -99,6 +99,8 @@ During compilation and publishing, these attributes are checked to ensure that:

From a storage perspective, a resource group is stored as a BCS-encoded `BTreeMap<StructTag, BCS encoded MoveValue>`, where a `StructTag` is a known structure in Move of the form: `{ account: Address, module_name: String, struct_name: String }`. Whereas, a typical resource is stored as a `BCS encoded MoveValue`.

Resource groups introduce a new storage access path: `ResourceGroup` to distinguish from existing access paths. This provides a cleaner interface and segration of different types of storage. This becomes advantageous to indexers and other direct readers of storage that can now parse storage without inspecting module metadata.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a bit lengthy but can we also clarify that since the current APIs only allow accessing specific resources via Resource access path or modules via Module access path, there's no way to access a specific resource group and load all of the contained resources. These individual resources, however, can be accessed via the list account resources API or by referring to each of these resources specifically. There are no use cases for fetching resource groups and they can be viewed as an internal data structure for smart contracts and VM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor

@banool banool left a comment

Choose a reason for hiding this comment

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

In the risks and drawbacks you mention the API layer. I figure you mean mostly the API <-> storage interface, rather than the data that the API serves? If not, it might be good to have more info there about what you expect the changes to look like.

3. Each entry within a resource group has a `resource_group` attribute.
4. The `container` parameter is set to a struct that is labeled as a `resource_group_container`.
5. During upgrade, an existing `struct` cannot either enter or leave a `resource_group`.
1. A `resource_group_member` has no abilities and no fields.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a typo right? The members must have fields to be useful? (as they do above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh, yes, this is resource_group


```move
#[resource_group(container = aptos_framework::object::ObjectGroup)]
#[resource_group_member(group = aptos_framework::object::ObjectGroup)]
struct Object has key {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this resource_group_member attribute is present, does it mean we'll enforce that these structs can only be used as part of that resource group, and not in any other resource group / on its own?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will make sure that is clear in the writing.

2. The `scope` within the `resource_group_member` can only become more permissive, that is it can either remain at a remain at the same level of accessibility or increase to the next.
3. Each entry within a resource group has a `resource_group_member` attribute.
4. The `group` parameter is set to a struct that is labeled as a `resource_group`.
5. During upgrade, an existing `struct` cannot either enter or leave a `resource_group_member`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I read the AIP but I'm still not sure why we have these constraints? I figure it's so we don't have to deal with backfilling / null data respectively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you'll end up in a situation where data is lost, I'll make that clearer. Specifically, if a struct enters a resource group any existing data will become inaccessible. Now one could theoretically add key to something after the fact and potentially make that into a resource group. But key + resource group go hand in hand.

@davidiw davidiw merged commit c6843ef into main Jan 16, 2023
@davidiw davidiw deleted the davidiw-patch-1 branch January 23, 2023 04:41
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