-
Notifications
You must be signed in to change notification settings - Fork 327
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 udc docs #526
Add udc docs #526
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.
Looking very good @andrew-fleming ! Left a couple of comments.
docs/modules/ROOT/pages/udc.adoc
Outdated
** <<reserved_address_space_for_deployer,Reserved Address Space for Deployer>> | ||
** <<reproducible_deployments,Reproducible Deployments>> | ||
* <<calculating_counterfactual_addresses,Calculating Counterfactual Addresses>> | ||
* <<universal_deployer_contract_api,Universal Deployer Contract API>> |
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 would use UDC API Specification
for consistency with other parts of the doc. A refactor to unify this pattern (and others) would be helpful IMO (out of scope for this PR).
Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.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.
great work! i've left some comments and suggestions, but overall very good and complete
docs/modules/ROOT/pages/udc.adoc
Outdated
@@ -0,0 +1,228 @@ | |||
= Universal Deployer Contract | |||
|
|||
Account contracts make up very critical components of the StarkNet ecosystem, since a bug in any implementation (let alone a widespread one) could prove disastrous. |
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.
this was the rationale to propose the UDC showcasing a use case but i'm not sure it's the same we need to explain what it is/how to use it once it's established.
i wouldn't focus that much in account account security but rather mention:
- it's a smart contract wrapping the deployment syscall to serve/support all other contracts that do not implement the function such as most account contracts
- it emits standard events for observability
- it aims to cover for the lack of a native deployment transaction type for accounts
Co-authored-by: Martín Triay <martriay@gmail.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.
i think we need to work a bit on the first section and it's done
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
Co-authored-by: Martín Triay <martriay@gmail.com>
Depends on #530 and #532.
Resolves #500. This PR follows the old API format (consistent with entire Contracts documentation); however, I strongly recommend updating to the API presentation proposed in OpenZeppelin/nile#298 in the future.