Add build and send sweep transaction for bitcoin js wallet #358

matthewjablack · 2020-09-30T05:47:48Z

Description

This PR adds buildSweepTransaction and sendSweepTransaction for Bitcoin js wallet enabling the ability to sweep all utxos to a destination of choice.

This is particularly useful if you wish to remove all funds from a wallet

Note: This can be useful for Ethereum as well. The major use case would be for sweeping all ETH from a wallet (since it's already pretty easy to do this for ERC20's)

Submission Checklist 📝

Add `buildSweepTransaction
Add `sendSweepTransaction
Add tests for sendSweepTransaction

monokh · 2020-09-30T14:34:30Z

Hey @matthewjablack thanks for the PR here, this is a very cool function
To keep the compatibility of the library uniform, we usually need any new functions to have implementations across ETH and ERC20 too, is this something you can add?

matthewjablack · 2020-09-30T23:40:46Z

Hey @monokh we're pretty focused on Bitcoin development at the moment, so unfortunately the ETH side of things would be out of scope of us.

monokh · 2020-10-02T21:24:53Z

@matthewjablack To ensure the library is predictable and fits the purpose of being an abstraction layer between the chains, we really need any functionality implemented to have a suitable interface and be implemented across the supported chains where possible.

Maintaining single chain based functionality adds overhead that the library is not suited towards. For example the current signature for async sendSweepTransaction (externalChangeAddress, outputs, fee, fixedInputs) is not compatible with the ETH chain and will need breaking changes when implemented cross chain.

So we ask for new features to be implemented cross chain because it:

Forces the right interface to be created at the client level
Ensures that the functionality is indeed able to be abstracted in a usable way
Ensures that between the chains, the function is uniform and behaves in a predictable way
Works as expected for the target users of the library (those looking to abstract cross chain)

matthewjablack · 2020-10-04T18:03:07Z

Hey @monokh good point about async sendSweepTransaction (externalChangeAddress, outputs, fee, fixedInputs) not being compatible with the ETH chain.

I've changed it to async sendSweepTransaction (externalAddress, fee, outputs, fixedInputs) and implemented it on the ETH chain

monokh

Please also include the ERC20 implementation. I believe it would be in EthereumErc20Provider

packages/bitcoin-wallet-provider/lib/BitcoinWalletProvider.js

monokh · 2020-10-05T08:54:10Z

packages/bitcoin-js-wallet-provider/lib/BitcoinJsWalletProvider.js

+      throw Error('There should not be any change for sweeping transaction')
+    }
+
+    _outputs.forEach((output, i) => {


What is going on here with the _outputs and fixedInputs ?
The common set of function across chains would be to send all value in the wallet to a single address.

Having _outputs and fixedInputs allows you to spend all utxos in a wallet, and send a specific amount to an address, while sending the change to another address.

Useful if you want to pay someone and sweep your wallet at the same time.

Ok, i see what you are going for here. However i think the functions should not cater for complex features, unless it can be abstracted cross chain or is required for a specific need.

we want to reduce our maintenance to what is necessary for common usage

changing that should make this function as well as getInputsForAmount simpler

There are actually a couple of use cases where this is quite important.

If you want to allow users to select specific utxos and spend them or send funds to a particular location (similar to Wasabi) having defined inputs and outputs is necessary.

Another case is for DLC's. Every DLC created, must commit to a specific utxo for the funding tx. If the user wants to create multiple DLC's, they must split up the utxos beforehand. To do this as efficiently as possible, it's important to be able to specify a particular utxo, create one output to be used for a specific DLC, and the other one be change.

packages/client/lib/Chain.js

packages/ethereum-js-wallet-provider/lib/EthereumJsWalletProvider.js

matthewjablack · 2020-10-05T23:26:29Z

@monokh added sendSweepTransaction to EthereumERC20Provider and addressed all your comments

monokh

This is looking in really good shape! Just one final comment

monokh · 2020-10-06T07:57:48Z

packages/bitcoin-js-wallet-provider/lib/BitcoinJsWalletProvider.js

+      throw Error('There should not be any change for sweeping transaction')
+    }
+
+    _outputs.forEach((output, i) => {


Ok, i see what you are going for here. However i think the functions should not cater for complex features, unless it can be abstracted cross chain or is required for a specific need.

monokh · 2020-10-06T08:00:12Z

packages/bitcoin-js-wallet-provider/lib/BitcoinJsWalletProvider.js

+      throw Error('There should not be any change for sweeping transaction')
+    }
+
+    _outputs.forEach((output, i) => {


we want to reduce our maintenance to what is necessary for common usage

monokh · 2020-10-06T08:11:07Z

packages/bitcoin-js-wallet-provider/lib/BitcoinJsWalletProvider.js

+      throw Error('There should not be any change for sweeping transaction')
+    }
+
+    _outputs.forEach((output, i) => {


changing that should make this function as well as getInputsForAmount simpler

packages/bitcoin-wallet-provider/lib/BitcoinWalletProvider.js

matthewjablack · 2020-10-07T07:04:17Z

Updated @monokh to remove Bitcoin specific functionality. This can be implemented externally if developers wish to allow wasabi type functionality

monokh

Looking good! Thanks for this

matthewjablack changed the title ~~Add build and send sweep transaction for js wallet~~ Add build and send sweep transaction for bitcoin js wallet Sep 30, 2020

Add build and send sweep transaction for js wallet

54dbc54

matthewjablack force-pushed the sweep-transaction branch from 63f1c37 to 54dbc54 Compare September 30, 2020 06:01

matthewjablack added 3 commits September 30, 2020 21:48

Adjust input output byte size calculation for fee per byte

7ea3709

Modify tests to ensure necessary funds when js wallet is sweeped

8689218

Change input and output fee calculation

e2d8caf

matthewjablack added 2 commits October 4, 2020 04:05

Add sendSweepTransaction to Ethereum js wallet

a33a5ff

Fix bitcoin tests for sweep tx

b7b9389

monokh requested changes Oct 5, 2020

View reviewed changes

matthewjablack added 4 commits October 5, 2020 19:17

Move logic of getInputsForSweep into getInputsForAmount for DRY

68355ac

Add sendSweepTransaction to Ethereum ERC20 Provider

c19dda5

Update Chain client to use cross-chain sendSweepTransaction interface

3100c57

Update ethereum js wallet provider sendSweepTransaction arg naming

075f4ec

monokh requested changes Oct 6, 2020

View reviewed changes

Remove Bitcoin specific input and output functionality for sweep

489db19

monokh self-requested a review October 7, 2020 07:35

monokh approved these changes Oct 7, 2020

View reviewed changes

monokh merged commit 2ff1b58 into dev Oct 7, 2020

snyk-bot mentioned this pull request Oct 8, 2020

[Snyk] Security upgrade @liquality/bundle from 0.3.9 to 0.7.0 #361

Closed

kraikov deleted the sweep-transaction branch April 29, 2022 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build and send sweep transaction for bitcoin js wallet #358

Add build and send sweep transaction for bitcoin js wallet #358

matthewjablack commented Sep 30, 2020 •

edited

Loading

monokh commented Sep 30, 2020

matthewjablack commented Sep 30, 2020 •

edited

Loading

monokh commented Oct 2, 2020 •

edited

Loading

matthewjablack commented Oct 4, 2020

monokh left a comment

monokh Oct 5, 2020

matthewjablack Oct 5, 2020

monokh Oct 6, 2020

monokh Oct 6, 2020

monokh Oct 6, 2020

matthewjablack Oct 6, 2020

matthewjablack commented Oct 5, 2020 •

edited

Loading

monokh left a comment

monokh Oct 6, 2020

monokh Oct 6, 2020

monokh Oct 6, 2020

matthewjablack commented Oct 7, 2020

monokh left a comment

Add build and send sweep transaction for bitcoin js wallet #358

Add build and send sweep transaction for bitcoin js wallet #358

Conversation

matthewjablack commented Sep 30, 2020 • edited Loading

Description

Submission Checklist 📝

monokh commented Sep 30, 2020

matthewjablack commented Sep 30, 2020 • edited Loading

monokh commented Oct 2, 2020 • edited Loading

matthewjablack commented Oct 4, 2020

monokh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjablack commented Oct 5, 2020 • edited Loading

monokh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjablack commented Oct 7, 2020

monokh left a comment

Choose a reason for hiding this comment

matthewjablack commented Sep 30, 2020 •

edited

Loading

matthewjablack commented Sep 30, 2020 •

edited

Loading

monokh commented Oct 2, 2020 •

edited

Loading

matthewjablack commented Oct 5, 2020 •

edited

Loading