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

utils: use container based cli #229

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

EmbeddedAndroid
Copy link
Contributor

@EmbeddedAndroid EmbeddedAndroid commented Oct 5, 2023

Summary of Changes

The util scripts currently are using host binaries for running commands against devnet. This can be confusing to users as we also provide wrappers for accessing the same binaries running inside the container devenv.

Some examples are:

https://github.com/stacks-network/sbtc/blob/main/devenv/bitcoin/bin/bitcoin-cli

https://github.com/stacks-network/sbtc/blob/main/devenv/sbtc/bin/sbtc

Implements: stacks-network/sbtc#224

This PR switches the utils scripts to use the wrappers for container binaries. For the binaries running inside devenv, it must use the service hostnames otherwise localhost will cause them to fail.

Testing

Risks

The biggest risk I see is, if the sbtc container (romeo) crashes, the sbtc command: https://github.com/stacks-network/sbtc/blob/main/devenv/sbtc/bin/sbtc will not be able to run, as the container stopped.

How were these changes tested?

Tested locally with devenv

What future testing should occur?

We need to automate the entire flow via stacks-network/sbtc#215

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@EmbeddedAndroid EmbeddedAndroid added the devenv Development Environment label Oct 5, 2023
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #229 (b8a3c9a) into main (9dabedd) will decrease coverage by 40.13%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #229       +/-   ##
===========================================
- Coverage   78.88%   38.76%   -40.13%     
===========================================
  Files           5       45       +40     
  Lines         270     5167     +4897     
  Branches       47       47               
===========================================
+ Hits          213     2003     +1790     
- Misses         56     3163     +3107     
  Partials        1        1               
Flag Coverage Δ
unittests 78.88% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 40 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@CAGS295 CAGS295 linked an issue Oct 5, 2023 that may be closed by this pull request
2 tasks
CAGS295
CAGS295 previously approved these changes Oct 5, 2023
friedger
friedger previously approved these changes Oct 5, 2023
Copy link
Collaborator

@friedger friedger 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

@EmbeddedAndroid
Copy link
Contributor Author

Hold on a moment, I need to update this PR for the merge conflict and one additional fix.

@friedger friedger dismissed stale reviews from CAGS295 and themself via 818c8e6 October 5, 2023 17:36
@friedger
Copy link
Collaborator

friedger commented Oct 5, 2023

I just fixed the merge conflicts. Sorry, It is your task, I guess. And @CAGS295 needs to review again :-)

@@ -12,4 +12,4 @@ fi

btc_address=$(source $dir/get_credentials.sh | jq -r '.credentials["1"].bitcoin.p2wpkh.address')

bitcoin-cli -rpcconnect=localhost -rpcport=18443 -rpcuser=devnet -rpcpassword=devnet generatetoaddress $num_blocks $btc_address
bitcoin-cli -rpcconnect=localhost -rpcport=18443 -rpcuser=devnet -rpcpassword=devnet generatetoaddress $num_blocks $btc_address
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we lose the options changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah let me sort this out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My fault 😓

@EmbeddedAndroid
Copy link
Contributor Author

Ok this is working for me, deploy_contracts, mine, deposit and withdrawal.

./up.sh
**wait for stacks to start mining**
./utils/deploy_contracts.sh
docker compose down sbtc
docker compose up sbtc -d
./utils/mine_btc.sh 200
./utils/deposit.sh
./utils/withdraw.sh

@CAGS295
Copy link
Contributor

CAGS295 commented Oct 5, 2023

@EmbeddedAndroid, how do you check that stacks-node is ready to deploy contracts? The heuristic I've used so far is checking for stacks height # 2.

@EmbeddedAndroid
Copy link
Contributor Author

@EmbeddedAndroid, how do you check that stacks-node is ready to deploy contracts? The heuristic I've used so far is checking for stacks height # 2.

that is what I'm doing, polling the api until I see stack height of 2+

@EmbeddedAndroid EmbeddedAndroid mentioned this pull request Oct 5, 2023
8 tasks
Copy link
Collaborator

@friedger friedger 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
Collaborator

@friedger friedger 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

@CAGS295 CAGS295 merged commit 3ec2c8a into stacks-network:main Oct 5, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devenv Development Environment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Utils scripts should run using devenv services
3 participants