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

Add udc docs #954

Merged
merged 54 commits into from
Apr 16, 2024
Merged

Add udc docs #954

merged 54 commits into from
Apr 16, 2024

Conversation

andrew-fleming
Copy link
Collaborator

@andrew-fleming andrew-fleming commented Mar 26, 2024

Fixes #950.

PR Checklist

  • Tests
  • Documentation
  • Added entry to CHANGELOG.md
  • Tried the feature on a public network

Ignore the line: "The UDC address is deployed at address 0xFIX-ME in ADD-NETWORKS-WHEN-DEPLOYED." This will be added when available.

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

Looking good! left some comments.

We should add Utilities,Universal Deployer Contract to page-sidebar-collapse-default: in antora.yml as well.

And the most important thing I think we should focus is being more explicit on the differences between UDC origin-dependent deployments, and deploy_syscall origin-dependent deployments, since currently it seems origin-dependent is specific to UDC while is not (from utilities names and wording).

src/tests/presets/test_universal_deployer.cairo Outdated Show resolved Hide resolved
src/utils/universal_deployer.cairo Outdated Show resolved Hide resolved
src/utils/universal_deployer.cairo Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/udc.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/udc.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/udc.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/udc.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/udc.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/udc.adoc Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/api/udc.adoc Outdated Show resolved Hide resolved
@andrew-fleming andrew-fleming marked this pull request as ready for review April 3, 2024 21:43
Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

Looking good andrew! Left some comments mostly about the helpers interface, and a small suggestion about the doc-site navbar organization.

src/utils/deployments.cairo Outdated Show resolved Hide resolved
salt: felt252,
class_hash: ClassHash,
constructor_calldata: Span<felt252>,
deployer_address: ContractAddress
Copy link
Member

@ericnordelo ericnordelo Apr 9, 2024

Choose a reason for hiding this comment

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

Wdyt about making this an Option instead of expecting zero?

If we do this I think we can convert the two udc helpers into just one, by receiving an Option<DeployerInfo>, being DeployerInfo (caller_address, udc_address).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Much better solution! Good call

src/utils/deployments.cairo Outdated Show resolved Hide resolved
@@ -38,6 +38,8 @@
*** xref:/api/upgrades.adoc[API Reference]

** xref:utilities.adoc[Utilities]
*** xref:udc.adoc[Universal Deployer Contract]
Copy link
Member

Choose a reason for hiding this comment

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

I feel this should not be under utilities, since we have a preset, I think this should be under the modules section directly, even if the interface is in the utilities.

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

I Left two small suggestions, but besides that it LGTM!

:utils-api: xref:/utilities.adoc#deployments[Utilities API]

This library offers utility functions written in Cairo to precompute contract addresses.
They include the generic {calculate_contract_address_from_deploy_syscall} as well as UDC-specific calculations.
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 link the udc util too?

/// Returns the contract address from a `deploy_syscall`.
/// `deployer_address` should be the zero address if the deployment is origin-independent (deployed from zero).
/// For more information, see https://docs.starknet.io/documentation/architecture_and_concepts/Smart_Contracts/contract-address/
fn calculate_contract_address_from_deploy_syscall(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn calculate_contract_address_from_deploy_syscall(
fn deploy_syscall_calculate_contract_address(

just for consistency with udc_calculate_contract_address

Copy link
Member

Choose a reason for hiding this comment

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

Small last comment, sorry since I was the one suggesting the change, but what do you think of putting calculate_contract_address at the beginning of the helper name? I think it feels better that way, since the most important part is that, no the deploy_syscall.

TL'DR: I vote we change deploy_syscall_calculate_contract_address to calculate_contract_address_from_deploy_syscall, and udc_calculate_contract_address to calculate_contract_address_from_udc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha no worries. Yeah, it seems more natural starting with calculate_. Will update

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

LGTM!

@andrew-fleming andrew-fleming merged commit afd683f into OpenZeppelin:main Apr 16, 2024
6 checks passed
@andrew-fleming andrew-fleming deleted the add-udc-docs branch April 16, 2024 22:53
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.

Add UDC docs
2 participants