refactor: run substrate-contracts-node in pop up contract if it does not exist

[AlexD10S]

• For pop up contract, check if the specified chain is live. If not, prompt the user to run the Substrate contracts node in the background for testing using pop-cli.
• This PR refactors the previous PR Add the "pop up contracts-node" command #185 by removing the pop up contracts-node command and reusing the logic to run the contracts-node exclusively for testing purposes. This improves the pop up contract user experience and will be reused for automatic execution in e2e functionality (Add CONTRACTS_NODE_PATH to Pop when running e2e tests for contracts #31).
• Improves the UX by downloading the binaries directly instead of building the node, following the approach Frank is implementing for pop up parachain in this PR feat(up parachain): faster binary sourcing #199

Closes #35

Reviewer Considerations

• The GitHub functionality is the same code as in the pop_parachains crate. I copied the necessary logic into the pop_contracts crate, but does it make more sense to just import the pop_parachains crate instead?
  Ideally, we should have a common crate as suggested here: Organization: crates/common #124, but that seems out of scope for this PR.
• The contracts node runs in the background, so after the contract deployment is finished, it remains running. The user will need to manually stop it.

[codecov]

Codecov Report

Attention: Patch coverage is 64.85149% with 71 lines in your changes missing coverage. Please review.

Project coverage is 57.31%. Comparing base (2713de5) to head (b2ba47c).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main     #206      +/-   ##
==========================================
+ Coverage   56.58%   57.31%   +0.72%     
==========================================
  Files          33       34       +1     
  Lines        3513     3697     +184     
  Branches     3513     3697     +184     
==========================================
+ Hits         1988     2119     +131     
- Misses       1232     1239       +7     
- Partials      293      339      +46     

Files	Coverage Δ
crates/pop-cli/src/main.rs	27.41% <ø> (+0.43%)	⬆️
crates/pop-contracts/src/errors.rs	0.00% <ø> (ø)
crates/pop-cli/src/commands/up/contract.rs	0.00% <0.00%> (ø)
crates/pop-contracts/src/utils/git.rs	78.82% <78.82%> (ø)
crates/pop-contracts/src/utils/contracts_node.rs	64.00% <64.00%> (ø)

[al3mart]

Nice!

Overall I think this flow is nicer.

Regarding that git duplicated code I agree we should move in the way of having a common crate, or maybe it makes sense importing it from the existing one just to be able to move ahead and then we address the common crate later on.

Given that we are launching a para for the user if no arguments are given, we could also use //Alice by default if no other suri is given.

When dry-running the changes I got:

◆  Local node started successfully in the background.
│  
▲  NOTE: The contracts node is running in the background with process ID 188188. Please close it manually when done testing.
│  
┌   Pop CLI : Deploy a smart contract
│
Error: Invalid number of input arguments: expected 1, 0 provided

I just built a contract and run pop up contract --suri //Alice. @AlexD10S did you see this ?

[Moliholy]

Given that we are launching a para for the user if no arguments are given, we could also use //Alice by default if no other suri is given.

@al3mart just some heads up: when using EthereumSignature instead of MultiSignature, //Alice does not work. Instead, Alith private key should be used as the uri.

You can check whether the blockchain uses EthereumSignature by checking the chainspec includes the isEthereum: true flag.

[AlexD10S]

Nice!

Overall I think this flow is nicer.

Regarding that git duplicated code I agree we should move in the way of having a common crate, or maybe it makes sense importing it from the existing one just to be able to move ahead and then we address the common crate later on.

Given that we are launching a para for the user if no arguments are given, we could also use //Alice by default if no other suri is given.

When dry-running the changes I got:

◆  Local node started successfully in the background.
│  
▲  NOTE: The contracts node is running in the background with process ID 188188. Please close it manually when done testing.
│  
┌   Pop CLI : Deploy a smart contract
│
Error: Invalid number of input arguments: expected 1, 0 provided

I just built a contract and run pop up contract --suri //Alice. @AlexD10S did you see this ?

The error thrown is correct. To initialize the contract, you must call the constructor method. If not specified, pop-cli uses 'new' by default. For the smart contract you are trying to deploy, this constructor expects one argument:

        #[ink(constructor)]
        pub fn new(init_value: bool) -> Self {

The argument has to be specified with the --args flag. If you try pop up contract --suri //Alice --args false works.
The same behaviour happens with cargo-contracts. If you try cargo contract instantiate --suri //Alice you will see the same error.

Also, I pushed a new commit implementing your suggestion to use the dev account by default (//Alice) if the account for deploying the contract is not specified (--suri).

[al3mart]

The error thrown is correct. To initialize the contract, you must call the constructor method.

So right! That was a bad review from my side. For whatever reason I started passing no arguments on my testing. 🙏

[AlexD10S]

The error thrown is correct. To initialize the contract, you must call the constructor method.

So right! That was a bad review from my side. For whatever reason I started passing no arguments on my testing. 🙏

You made a good point; the error message is not very clear. However, I'm unsure how we can improve it because the error is thrown by the contract_extrinsics. We could add a check in pop-cli to retrieve the constructor and compare the expected arguments.

[AlexD10S]

@al3mart just some heads up: when using EthereumSignature instead of MultiSignature, //Alice does not work. Instead, Alith private key should be used as the uri.

You can check whether the blockchain uses EthereumSignature by checking the chainspec includes the isEthereum: true flag.

In my opinion, this is out of scope for this PR. We are safe using //Alice if no --suri is indicated. This is just a default scenario. If the chain has EthereumSignature, the user will have to specify the keys they want to use. But thanks for sharing, please feel free to add it to the issues, and we will consider adding this feature..

[al3mart]

LGTM