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

chore: remove chainhook subcommands #1328

Merged

Conversation

tippenein
Copy link
Collaborator

@tippenein tippenein commented Jan 23, 2024

closes #1308

(also fixes #1310 links)

@sabbyanandan
Copy link
Contributor

@tippenein: 💥!

More importantly, I love this:
image

@tippenein tippenein force-pushed the remove-chainhook-subcommands-1308 branch from 6bca7b7 to 7adee9a Compare January 23, 2024 19:44
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4bfe976) 40.44% compared to head (b6347ae) 40.64%.

Files Patch % Lines
components/clarinet-cli/src/frontend/cli.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1328      +/-   ##
===========================================
+ Coverage    40.44%   40.64%   +0.20%     
===========================================
  Files           87       86       -1     
  Lines        31766    31591     -175     
===========================================
- Hits         12847    12841       -6     
+ Misses       18919    18750     -169     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hugocaillard
Copy link
Collaborator

Thanks @tippenein. And happy new year! 🎉

What do you think about printing a message explaining that clarinet chainhook is no longer available (because it wasn't maintained)? And maybe suggest alternatives? (cc @MicaiahReid)

No hurry on this, I think we'll hold the PR for after the next release (2.2.0)

@tippenein
Copy link
Collaborator Author

hey @hugocaillard
Yeah that sounds fine by me 👌

Copy link
Contributor

@MicaiahReid MicaiahReid left a comment

Choose a reason for hiding this comment

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

Hey @tippenein, thanks for the PR!

I had some time to give this one some thought, and I think we only actually want part of this change.

Clarinet had the clarinet chainhook new command that would generate chainhooks that are no longer compatible with the actual Chainhook tool. Now that Chainhook is a standalone tool, we don't need to have the generation of these files inside of Clarinet.

However, Clarinet still finds a user's chainhooks in their working directory and automatically registers them with their devnet, which is a really helpful feature that we actually just released a guide on.

Would you be able to revert the changes that remove chainhook from the devnet?

components/clarinet-cli/src/frontend/cli.rs Outdated Show resolved Hide resolved
components/clarinet-cli/src/devnet/start.rs Outdated Show resolved Hide resolved
components/stacks-network/src/chainhooks.rs Show resolved Hide resolved
components/stacks-network/src/chains_coordinator.rs Outdated Show resolved Hide resolved
components/stacks-network/src/lib.rs Show resolved Hide resolved
components/stacks-network/src/main.rs Outdated Show resolved Hide resolved
@tippenein
Copy link
Collaborator Author

@MicaiahReid thanks for the feedback. So, we're essentially only deprecating the chainhook new ?

This is what it looks like currently:
Screenshot 2024-02-08 at 2 36 05 PM
Screenshot 2024-02-08 at 2 36 28 PM

@MicaiahReid
Copy link
Contributor

So, we're essentially only deprecating the chainhook new

@tippenein Apologies for the mixed messaging here. We can also deprecate chainhook check and chainhook deploy.
If, after deprecating those, you find some helper functions in the stacks-network component that are no longer used, those can be removed (for example, I think we'll be able to do away with the check_chainhooks function).

What will need to remain is all code that is used by the stacks-network to load the chainhooks into a devnet.

@tippenein tippenein force-pushed the remove-chainhook-subcommands-1308 branch from 72181b6 to bea312a Compare February 9, 2024 20:52
@tippenein
Copy link
Collaborator Author

@MicaiahReid Yea that makes total sense. It should be good now 👍 lmk

Copy link
Contributor

@MicaiahReid MicaiahReid left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @tippenein!

@hugocaillard
Copy link
Collaborator

Thanks @tippenein!
I'm merging now

@hugocaillard hugocaillard merged commit 1d084ef into hirosystems:develop Feb 12, 2024
19 checks passed
@tippenein tippenein deleted the remove-chainhook-subcommands-1308 branch February 12, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Link broken in helper of terminating devnet update or remove chainhook commands
4 participants