Skip to content
This repository was archived by the owner on Dec 4, 2024. It is now read-only.

Conversation

@EmbeddedAndroid
Copy link
Contributor

Summary of Changes

As discussed, we want to make it easier to fund the stacks deployer wallet.

Premine both the stacks miner wallet and stacks deployer wallets, and continue to mine to the deplorer's wallet.

This PR depends on stacks-sbtc/sbtc#229

Testing

Risks

The risk is low, as this was tested end to end with devenv

How were these changes tested?

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

What future testing should occur?

Automation of end to end deposit and withdraw flows.

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

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

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

@@             Coverage Diff             @@
##             main     #237       +/-   ##
===========================================
- 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
Copy link
Contributor

CAGS295 commented Oct 5, 2023

Can we reiterate what accounts need funding for regular sBTC operation? Also, @friedger, can you refresh your concern about nonce issues?

Here is what I understood.

  1. Mining empty blocks with the developer address will cause nonce issues.
  2. The developer address requires initial funds but not necessarily mining continuously.
  3. It would be good to stick with the current miner address; I forgot why.

@friedger
Copy link
Contributor

friedger commented Oct 5, 2023

My understanding

btc
Deployer does not need btc

Alice needs btc.

Stbc wallet needs btc for fees if withdrawal request do not include enough fees.

Stacks miner needs btc.

stx
Deployer needs stx (funded on startup)

Alice does not need stx

Sbtc wallet does not need stx, stx txs are send from deployer.

Stx miner does not need stx.

Re nonce issue, I can't find an argument, why nonce issues might occur. Maybe we just ignore my comment.

@CAGS295
Copy link
Contributor

CAGS295 commented Oct 5, 2023

My understanding

btc Deployer does not need btc

Alice needs btc.

Stbc wallet needs BTC for fees if withdrawal requests do not include enough fees.

Stacks miner needs btc.

stx Deployer needs stx (funded on startup)

Alice does not need stx

Sbtc wallet does not need stx; stx txs are sent from deployer.

Stx miner does not need stx.

Re nonce issue, I can't find an argument why nonce problems might occur. Maybe we should just ignore my comment.

After digesting your comments, I think it would be best to drop this PR and add an --address option to the mine_btc.sh. The way utils scripts are used is an integration test flow. So we can go further and hardcode it instead of having --address.


tx=$(echo -n $json | jq -r .hex)

sbtc broadcast localhost:60401 $tx | jq -r .
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this work before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this pr depends on stacks-sbtc/sbtc#229

Copy link
Contributor

@friedger friedger Oct 6, 2023

Choose a reason for hiding this comment

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

We all had installed the sbtc binary locally..

@EmbeddedAndroid
Copy link
Contributor Author

My understanding
btc Deployer does not need btc
Alice needs btc.
Stbc wallet needs BTC for fees if withdrawal requests do not include enough fees.
Stacks miner needs btc.
stx Deployer needs stx (funded on startup)
Alice does not need stx
Sbtc wallet does not need stx; stx txs are sent from deployer.
Stx miner does not need stx.
Re nonce issue, I can't find an argument why nonce problems might occur. Maybe we should just ignore my comment.

After digesting your comments, I think it would be best to drop this PR and add an --address option to the mine_btc.sh. The way utils scripts are used is an integration test flow. So we can go further and hardcode it instead of having --address.

Happy to close, is this the direction we want to go.

Copy link
Contributor

@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

The setup about where and when BTCs are mined should be documented in https://github.com/stacks-network/sbtc-docs/blob/master/src/sbtc-releases/sbtc-dev/get-started-on-devnet.md

@EmbeddedAndroid
Copy link
Contributor Author

LGTM

The setup about where and when BTCs are mined should be documented in https://github.com/stacks-network/sbtc-docs/blob/master/src/sbtc-releases/sbtc-dev/get-started-on-devnet.md

I'll open a PR for this today.

@CAGS295
Copy link
Contributor

CAGS295 commented Oct 6, 2023

@friedger

My understanding

btc Deployer does not need btc

Stbc wallet needs BTC for fees if withdrawal requests do not include enough fees.

Why are we changing the miner to fund the deployer if the deployer does not need btc?
We should fund sbtc wallet instead. Alice is already being funded in mine_btc.sh. It is better to explicitly request funding for the sbtc wallet in the script to accommodate its requirements. Let's keep things tidy. We are stretching too far. If we were to delete mine_btc.sh, the miner service would be left with stray logic. 'mine_btc.sh' is not a tool nor a test; it is something in between for now. Let's keep things simple, please.

@friedger
Copy link
Contributor

friedger commented Oct 6, 2023

Let's move the changes to funding into a new pr

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants