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 declare script for presets #1118

Merged
merged 22 commits into from
Oct 8, 2024

Conversation

andrew-fleming
Copy link
Collaborator

@andrew-fleming andrew-fleming commented Aug 27, 2024

This PR proposes to add a sncast script that declares all the preset contracts. This script aims to improve the releasing process.

PR Checklist

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

Copy link
Collaborator

@immrsd immrsd left a comment

Choose a reason for hiding this comment

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

Good job! Left a couple of thoughts

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.87%. Comparing base (6837fe1) to head (956a586).
Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1118   +/-   ##
=======================================
  Coverage   88.87%   88.87%           
=======================================
  Files          57       57           
  Lines        1375     1375           
=======================================
  Hits         1222     1222           
  Misses        153      153           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6837fe1...956a586. Read the comment docs.

@andrew-fleming andrew-fleming marked this pull request as ready for review September 24, 2024 07:21
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.

Nice start @andrew-fleming. I wonder why is this located inside the presets package being a housekeeping improving script. I would move it out of it, maybe into a sncast_scripts directory on the root of the project, that won't be part of the released workspace.

Copy link
Collaborator

@immrsd immrsd left a comment

Choose a reason for hiding this comment

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

Good job! Left a couple of suggestions and questions

sncast_scripts/src/declare_presets/declare_presets.cairo Outdated Show resolved Hide resolved
sncast_scripts/src/declare_presets/declare_presets.cairo Outdated Show resolved Hide resolved
sncast_scripts/src/declare_presets/declare_presets.cairo Outdated Show resolved Hide resolved
sncast_scripts/src/declare_presets/declare_presets.cairo Outdated Show resolved Hide resolved
sncast_scripts/src/declare_presets/declare_presets.cairo Outdated Show resolved Hide resolved
sncast_scripts/src/declare_presets/declare_presets.cairo Outdated Show resolved Hide resolved
Co-authored-by: immrsd <103599616+immrsd@users.noreply.github.com>
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-fleming. Left some comments.

Comment on lines +28 to +31
--account path/to/account.json \
--keystore path/to/key.json \
script run declare_presets \
--url http://127.0.0.1:5050
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
--account path/to/account.json \
--keystore path/to/key.json \
script run declare_presets \
--url http://127.0.0.1:5050
script run declare_presets \
--account path/to/account.json \
--keystore path/to/key.json \
--url http://127.0.0.1:5050

Copy link
Member

Choose a reason for hiding this comment

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

Having options after the command feels more natural.

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 agree, but sncast only accepts the account flag before the command

@@ -0,0 +1,25 @@
[package]
name = "sncast_scripts"
version = "0.17.0"
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
version = "0.17.0"
version = "0.1.0"

Does this need to be pegged to the latest release from the library?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmm no, probably not. Good call

@@ -0,0 +1 @@
mod declare_presets;
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we have the script code in here? Do we need the extra directory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, updated!

Copy link
Collaborator

@immrsd immrsd left a comment

Choose a reason for hiding this comment

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

LGTM!

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 c5cd715 into OpenZeppelin:main Oct 8, 2024
8 checks passed
@andrew-fleming andrew-fleming deleted the add-declare-script branch October 8, 2024 20:48
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.

3 participants