-
Notifications
You must be signed in to change notification settings - Fork 102
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
Improve modref docs #1256
Improve modref docs #1256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes provide value and make it much better to read.
I'm not sure about the _TODO
sections as they require future efforts, which might lead to inconsistent documentation.
@rsoeldner @jwiegley I rewrote a lot of the sections with code examples and removed the TODOs. I do mention pinned modrefs as a possible future feature, and expanded the vuln description. Do you mind re-reviewing now? |
Are the rst files automatically generated or hand curated? It looks like the last commits special modifications. |
Automatically (manually triggered) -- |
and should not be confused with more object-oriented polymorphism like that found | ||
with Java classes or Typescript types. Modules cannot "extend" one another, they | ||
can only offer operations that match some interface specification, and interfaces | ||
themselves cannot extend some other interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a much clearer paragraph now 👍
directly call `pay-fee`. Thus a malicious actor can craft a version of `collect` | ||
that can directly call `pay-fee` as many times as they like. | ||
The problem with the above code is that the `with-capability` call happens | ||
_before_ the calls to the modref operations, such that while the foreign module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the 'foreign module' terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful!
Co-authored-by: Thomas Honeyman <hello@thomashoneyman.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Just docs, no downstream impacts or code coverage requirements.