CLI command to submit a deposit sweep proposal #3549
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The function can be used to validate the proposal with bitcoin and ethereum chains.
We added a CLI command to submit a deposit sweep proposal. The command verifies the proposal with the same rules as nodes will do and submits the proposal to the chain. To submit a deposit sweep proposal run: ``` keep-client wallet sweep --wallet <wallet_address> --fee <fee> <btc_tx_1> <btc_tx_2> ... ``` Bitcoin transactions are expected to be provided as variadic arguments in the following format: `<transaction hash>:<output index>` Sample command: ``` keep-client wallet sweep --wallet 0x03b74d6893ad46dfdd01b9e0e3b3385f4fce2d1e --fee 12 bd99d1d0a61fd104925d9b7ac997958aa8af570418b3fde091f7bfc561608865:1 178628f0fd77ca04a31e771805277405288404da38eb86a0e369f2b26d45fe97:1 b71c0bbe43f23e162739b80b7bae9f0fb650e90e56fea0b92f169553505c398d:1 ```
We want to make the flag required only in sweep command so we define separate flags for deposits and sweep commands.
If the --dry-run flag is used the command will validate the proposal but not submit the proposal to the chain.
There was a problem hiding this comment.
Just a quick pass. I'll do a thorough review once #3545 is merged.
cmd/wallet.go
Outdated
| @@ -68,6 +71,70 @@ var depositsCommand = cobra.Command{ | |||
| }, | |||
| } | |||
|
|
|||
| var sweepCommand = cobra.Command{ | |||
| Use: "sweep", | |||
There was a problem hiding this comment.
Can we name this command propose-deposit-sweep? First, this command just submits the proposal and sweeping is purely a wallet action. Second, in the future, we will also have "moved funds sweep" so it would be good to make it specific that we are dealing with "deposit sweep" here.
There was a problem hiding this comment.
Maybe propose-sweep to keep it shorter?
There was a problem hiding this comment.
Remember that we will also have "propose moved funds sweep". Unfortunately, we must be specific here and make the command a bit longer.
There was a problem hiding this comment.
If there will be more we could make the sub-commands nested, e.g.:
deposits list
deposits sweep propose
deposits sweep bump-fee
deposits redeem
redemptions list
moved-funds list
moved-funds sweep propose
moved-funds sweep bump-fee
But for now, I updated it the way that was proposed: 31b7d30
There was a problem hiding this comment.
Yeah, good idea regarding the sub-commands! Leaving this discussion open for future reference.
The names are now more descriptive.
This assignment was missing.
Since keep-network/tbtc-v2#613 deposit sweep proposal requires a block numebr of the reveal deposit transaction. We need to output the data in the deposits list and pass it to the propose sweep function.
We should use the value stored in the chain implementation, as it gets overwritten in test.
Couple of modifications to the function.
pdyraga
reviewed
May 9, 2023
For easier maintanance of the format and examples in the code we defined variables and used them in various places.
The propery is passed directly to the contract. The contract expects value for the whole transaction in satoshi.
lukasz-zimnoch
approved these changes
May 9, 2023
There was a problem hiding this comment.
Works as advertised! I used the following command:
./keep-client --config ./configs/config.coordinator.toml \
--goerli \
coordinator \
propose-deposits-sweep \
--wallet 0x03b74d6893ad46dfdd01b9e0e3b3385f4fce2d1e \
--fee 10000 \
4ef58d68e35783695d898786bb29432be32325b1bb2cc6a9cf58c622cf2a1e94:0:8381106 \
167d96e203fc91d014026ab2ffa9e44b6a2a425c22febd9bffe88b6dddca1979:0:8381194 \
63cb18fdbe4ceb8b0747dbfcb11989605f78cffbe4379b9047124118cfc20aef:0:8381671 \
46a9b50dd44f11364d9924bd7995830cd7cd1a772910f22ed7ab507137653c6d:0:8381974 \
cec3d569224ebeeaf7aef922fbac498c4c75633529acff28d50a94cac332e87c:0:8382422
and the sweep proposal was submitted as expected: https://goerli.etherscan.io/tx/0x2c9d6c3f865b5aa52080ecea25a1c75f3fdc0401dccaa6987f4c8ced5e85e8cd
pdyraga
added a commit
that referenced
this pull request
Jun 14, 2023
The changes here are a step on the road to the fully automated sweeps (#3614). We update the `propose-deposits-sweep` command initially implemented in #3549. The main change to the command is that if deposits are not provided as arguments, they will be automatically fetched from the chain. The command will find the oldest unswept deposits for the oldest unswept wallet and construct a sweep proposal for them. Number of deposits for the proposal is based on the [`depositSweepMaxSize`](https://github.com/keep-network/tbtc-v2/blob/66a04c618e140230d25ea0d784ed75e6ba3030a2/solidity/contracts/bridge/WalletCoordinator.sol#L191) defined in the `WalletCoordinator` contract. The value can be overwritten with `--deposit-sweep-max-size <number>`. If `--wallet <wallet-public-key-hash>` is provided the command will find unswept deposits for the particular wallet. If `--fee <number>` is not provided the command will estimate a fee for the sweep transaction. ## Sample Command To execute the command in dry-run mode: ```sh # Sweep deposits from the oldest unswept wallet: LOG_LEVEL=debug go run . coordinator --testnet propose-deposits-sweep --dry-run # Sweep deposits from a specific wallet: LOG_LEVEL=debug go run . coordinator --testnet propose-deposits-sweep --wallet 0x7670343fc00ccc2d0cd65360e6ad400697ea0fed --dry-run ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We added a CLI command to submit a deposit sweep proposal. The command verifies the proposal with the same rules as nodes will do and submits the proposal to the chain.
To submit a deposit sweep proposal run:
Following flags can be used with the command:
--wallet <address>- wallet to sweep--fee- fee to use for the sweep transactionThe command requires
ethereumandbitcoin.electrumnode details to be provided in the config.Deposits
The command requires the deposits details to be provided as variadic arguments in a specific format:
<unprefixed bitcoin transaction hash>:<bitcoin transaction output index>:<ethereum reveal block number>e.g.
bd99d1d0a61fd104925d9b7ac997958aa8af570418b3fde091f7bfc561608865:1:8392394Sample command
Testing
To test this featureWalletCoordinatorcontract is required to be deployed. It is not yet available on Goerli or Mainnet. To have the contract available you can run a hardhat node as Goerli fork (https://github.com/nkuba/hardhat-forked-node) and deploy theWalletCoordinatorcontract there.Depends on: #3545