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 presets usage #949

Merged
merged 8 commits into from
Mar 28, 2024
Merged

Conversation

andrew-fleming
Copy link
Collaborator

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

Fixes #839.

PR Checklist

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

@andrew-fleming andrew-fleming marked this pull request as ready for review March 21, 2024 19:38
:install_lib: xref:index.adoc#install the_library[installing the Contracts for Cairo library]
:presets_dir: link:https://github.com/OpenZeppelin/cairo-contracts/blob/release-v0.10.0/src/presets[presets directory]

These preset contracts are ready-to-deploy which means they should already be declared on the Sepolia network.
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 something we have failed to guarantee, we should probably add it to our development/release process. It's partially covered by our the PR process (also not fully applied) requiring that new features are tested onchain, but that's not necessarily true for the final versions that get released (just like a classhash might end up being different once merged to main)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is something we have failed to guarantee, we should probably add it to our development/release process

Agreed. I added the step in our Release process

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the future, I think it'd be nice to write a script for declaring the presets to make this even easier. Maybe we can leverage scarb's scripts for this e.g. scarb run declare-presets

Copy link
Contributor

Choose a reason for hiding this comment

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

the hard part is to automate the payments, but yes

--network="sepolia"
----

If a class hash has yet to be declared, copy/paste the preset contract code and declare it locally.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add more detail/an example of how copypasting should be done?
can we do something better than copypasting? distributing built artifacts?

Copy link
Member

Choose a reason for hiding this comment

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

My two cents fwiw, copypasting is consistent with the expected flow in Wizard, and distributing built artifacts for a thing you declare just once seems like an overhead, more when we favor wizard long term instead of local presets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I take the point, but i still think copypasting can be error prone and it doesn't feel quite right for it to be the recommended way of using code. even wizard offers downloading the files which imo is way better.

as of the overhead, we could offer artifacts as a submodule/separate module, so i don't think it'd be that much of a burden if properly automated.

i guess we can iterate it over time, from copypasting to something a bit more reliable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I take the point, but i still think copypasting can be error prone and it doesn't feel quite right for it to be the recommended way of using code.

Since the class hashes should be declared (as part of the new release process), users shouldn't have to do any of this. I do get the apprehension for c/p though. I'll add an example

as of the overhead, we could offer artifacts as a submodule/separate module, so i don't think it'd be that much of a burden if properly automated.
i guess we can iterate it over time, from copypasting to something a bit more reliable.

+1

docs/modules/ROOT/pages/presets.adoc Outdated Show resolved Hide resolved
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! I think we should somehow mention Wizard in here, encouraging people to use the tool instead of the presets directly.

@martriay
Copy link
Contributor

Looking good! I think we should somehow mention Wizard in here, encouraging people to use the tool instead of the presets directly.

+1 to wizard, but it's not like we want to discourage preset usage

@andrew-fleming
Copy link
Collaborator Author

Looking good! I think we should somehow mention Wizard in here, encouraging people to use the tool instead of the presets directly.

+1 to wizard

We already have it mentioned at the top of the page as a TIP. You think it's worth repeating in Usage?

@martriay
Copy link
Contributor

no

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 dac0de3 into OpenZeppelin:main Mar 28, 2024
5 checks passed
@andrew-fleming andrew-fleming deleted the add-preset-usage branch March 28, 2024 17:05
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.

Write preset usage guide
3 participants