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

Documentation for entitlements #152

Merged
merged 24 commits into from
Jul 18, 2023
Merged

Documentation for entitlements #152

merged 24 commits into from
Jul 18, 2023

Conversation

dsainati1
Copy link
Contributor

No description provided.

@dsainati1 dsainati1 added the documentation Improvements or additions to documentation label Jun 26, 2023
@dsainati1 dsainati1 self-assigned this Jun 26, 2023
@vercel
Copy link

vercel bot commented Jun 26, 2023

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 18, 2023 3:52pm

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

The docs are very thorough, but they are quite academic. It would be great to have a more easily accessible tutorial or walkthrough of entitlements in addition to what we have here

Comment on lines 52 to 58
- Entitled access means the declaration is only accessible/visible
to owned values, or to references that are authorized to the required entitlements.

For example, an `access(E, F)` field can only be accessed by an owned value,
or a reference to a value that is authorized to the `E` and `F` entitlements.

An element is made accessible by code in the same containing type
Copy link
Member

Choose a reason for hiding this comment

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

I think we might need to be a little more hand-holdy with this part. A lot of people come here for their first introduction to access control in cadence, so using terms like "owned values" or "references that are authorized to the required entitlements" without any context is going to really confuse people who aren't familiar with anything in Cadence. I'd almost rather just avoid talking about entitlements at all in this section since entitlements are an advanced concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I agree that we could make this presentation simpler, but I don't think we can avoid talking about entitlements here. They aren't really an advanced concept; they are pretty fundamental to how access control is going to work in Cadence. It's going to be essentially impossible to write a contract post-Stable Cadence without understanding how entitlements work at least at a basic level, so I think we need to have this section present an introduction to the concept at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some additional language about what an "owned value" is and what it means for a reference to be authorized to an entitlement.

docs/cadence/language/access-control.md Outdated Show resolved Hide resolved
docs/cadence/language/access-control.md Outdated Show resolved Hide resolved
docs/cadence/language/access-control.md Outdated Show resolved Hide resolved
docs/cadence/language/references.mdx Outdated Show resolved Hide resolved
Co-authored-by: Joshua Hannan <hannanjoshua19@gmail.com>
@dsainati1
Copy link
Contributor Author

It would be great to have a more easily accessible tutorial or walkthrough of entitlements in addition to what we have here

Agreed 1000%. I think the current plan is to rewrite all the existing tutorials to use entitlements (they're all going to be outdated by Stable Cadence regardless), so hopefully seeing concrete examples of entitlements in use for those will be valuable.

docs/cadence/language/access-control.md Outdated Show resolved Hide resolved
docs/cadence/language/access-control.md Outdated Show resolved Hide resolved
@dsainati1
Copy link
Contributor Author

cc @SupunS can you take a look at these and let me know what you think? We can update them with your entitlements changes too once those are all merged.

Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

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

Nice! just some minor suggestions

docs/cadence/language/access-control.md Show resolved Hide resolved
docs/cadence/language/access-control.md Show resolved Hide resolved
docs/cadence/language/access-control.md Outdated Show resolved Hide resolved
docs/cadence/language/access-control.md Outdated Show resolved Hide resolved
let alsoEntitledSubRef = r.getRef()
```

{/* TODO: Update once mappings can be used on regular composite fields */}
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this a comment?

Suggested change
{/* TODO: Update once mappings can be used on regular composite fields */}
<!-- TODO: Update once mappings can be used on regular composite fields -->

Copy link
Member

Choose a reason for hiding this comment

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

This wasn't addressed/fixed

docs/cadence/language/attachments.mdx Outdated Show resolved Hide resolved
docs/cadence/language/references.mdx Outdated Show resolved Hide resolved
Co-authored-by: Supun Setunga <supun.setunga@gmail.com>
Base automatically changed from sainati/remove-pub-priv-docs to main July 18, 2023 14:47
@dsainati1 dsainati1 merged commit 4a21588 into main Jul 18, 2023
2 checks passed
@dsainati1 dsainati1 deleted the sainati/entitlement-docs branch July 18, 2023 15:57
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

docs/cadence/language/access-control.md Show resolved Hide resolved
docs/cadence/language/access-control.md Show resolved Hide resolved
let alsoEntitledSubRef = r.getRef()
```

{/* TODO: Update once mappings can be used on regular composite fields */}
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't addressed/fixed

Entitlement mappings may be used either in accessor functions (as in the example above), or in fields whose types are references. Note that this latter
usage will necessarily make the type of the composite non-storage, since it will have a reference field.

{/* TODO: once the Account type refactor is complete and the documentation updated, let's link here to the Account type page as an example of more complex entitlement mappings */}
Copy link
Member

Choose a reason for hiding this comment

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

Same here: turn into a Markdown comment, i.e. <!-- ... -->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I tried doing this, it made the CI complain. I'm pretty sure these are comments as-is, since they don't show up here: https://developers.flow.com/next/cadence/language/access-control

docs/cadence/language/access-control.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants