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 the "pop up contracts-node" command #185

Merged
merged 3 commits into from
May 22, 2024

Conversation

Moliholy
Copy link
Contributor

@Moliholy Moliholy commented May 20, 2024

Description

This PR adds the following command that spawns a substrate-contracts-node instance.

pop up contracts-node

It should be executed in a different terminal, and prior to its execution it will check whether the repository is downloaded and build the binary if it isn't.

It's definitely not rocket science, and could be improved, but IMO it's a decent step forward towards a better usage by integrating in the tool a component that was excluded, yet it was used for deploying smart contracts.

Possible improvements

  • Check whether the current version of the software is the latest, and if not force-download the latest.
  • The above, but with a --force-like command.
  • Include features when building the binary.
  • Pass command-line arguments to the node for more sophisticated setup.

@Moliholy Moliholy force-pushed the contracts-node branch 3 times, most recently from b93c66c to efe641b Compare May 20, 2024 23:27
@AlexD10S
Copy link
Collaborator

Hola @Moliholy! Great to see you here contributing to pop-cli. Looks good to me!

We are currently having an open discussion about the best UX for pop up: #129
I like pop up contracts-node , but want to see @brunopgalvao thoughts on it.

Also after this PR is merged, we can tackle this issue: #35

@Moliholy
Copy link
Contributor Author

@AlexD10S pleased to be around :-)

Yes, I saw that issue before, but I supposed its decision was not taken since it was still open, so I thought the best approach would be, at least for now, to continue with the current project's structure.

As for the name itself, I definitely don't have a strong opinion nor I'm particularly good at naming, so if you find a better one feel free to change it.

Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

Just added a little change in a message but the rest looks good to me!

Thanks again for contributing!!

crates/pop-cli/src/commands/up/contracts_node.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 50.10%. Comparing base (028150b) to head (5f381da).

@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
- Coverage   50.41%   50.10%   -0.32%     
==========================================
  Files          32       33       +1     
  Lines        2858     2876      +18     
  Branches     2858     2876      +18     
==========================================
  Hits         1441     1441              
- Misses       1194     1212      +18     
  Partials      223      223              
Files Coverage Δ
crates/pop-cli/src/main.rs 26.98% <0.00%> (-0.44%) ⬇️
crates/pop-cli/src/commands/up/contracts_node.rs 0.00% <0.00%> (ø)

@Moliholy
Copy link
Contributor Author

Rebased to the latest main.

@AlexD10S AlexD10S self-requested a review May 22, 2024 17:21
@AlexD10S AlexD10S merged commit c2aaa08 into r0gue-io:main May 22, 2024
12 of 14 checks passed
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