Append nullable withdrawalTxId field to Trade proto message #4937
Conversation
BtcWalletService was changed to allow the api to override tx fee rates from the sendbsq and sendbtc methods. The api methods will still be able to use the network fee service and custom tx fee rate preference, and set / unset the custom tx fee rate preference, but the change will permit the addition of an optional txFeeRate parameter to the sendbsq and sendbtc methods (todo). A few other minor changes (style and removal of never thrown ex spec) were also made to this class. Two BtcWalletService methods were refactored. - The redundant (was always true) boolean isSendTx argument was removed from the completePreparedVoteRevealTx method signature. - The redundant (was always true) boolean useCustomTxFee was removed from the completePreparedBsqTx method signature. - The completePreparedSendBsqTx method was overloaded with a 2nd parameter (Coin txFeePerVbyte) to allow api to override fee service and custom tx fee rate when sending BSQ or BTC. - The completePreparedBsqTx method was overloaded with a 3rd parameter (Coin txFeePerVbyte) to allow api to override fee service and custom tx fee rate when sending BSQ or BTC. The following line was deleted from the completePreparedBsqTx method because txFeePerVbyte is now an argument: Coin txFeePerVbyte = useCustomTxFee ? getTxFeeForWithdrawalPerVbyte() : feeService.getTxFeePerVbyte(); This useCustomTxFee value was always true, and redudant here because getTxFeeForWithdrawalPerVbyte() returns feeService.getTxFeePerVbyte() or the custom fee rate preference. i.e., Coin txFeePerVbyte = useCustomTxFee ? getTxFeeForWithdrawalPerVbyte() : feeService.getTxFeePerVbyte(); is equivalent to Coin txFeePerVbyte = getTxFeeForWithdrawalPerVbyte(); LockupTxService, UnlockTxService, BsqSendView, and BsqTransferService were adjusted to this BtcWalletService refactoring.
If present in the sendbsq command, the parameter will override the fee service and custom fee rate setting for the BSQ transaction. Also changed the sendbsq grpc return type to a lightweight TX proto wrapper. Besides some small refactoring in the CLI, all the changes are adjustments for this new sendbsq parameter and its new grpc return value.
Takes an address, amount, and optional txfeerate param, returns a lightweight TxInfo proto. Also overloaded two BtcWalletService methods to allow sendbtc to pass in the tx fee rate -- overriding the fee service and custom fee rate setting.
- Added optional memo parameter to the api's sendbtc and withdrawfunds commands. - Removed the @nullable annotation was removed because protobuf does not support null. - Visibility in two wallet check methods were changed from private to pkg protected so the CoreTradeService could use them. - Adjusted affected tests. (Asserting the memo field was set on a transaction cannot be checked from apitest yet.)
The withdrawalTxId field will be set in TradeManager#onWithdrawRequest upon successful trade completion and withdrawal of funds. Persisting withdrawalTxId will allow the api and ui to find the withdrawalTxId for a completed trade after the seller withdraws funds to an external wallet. In turn, the withdrawal tx's memo field will be accessible in a new (todo) api getTx(txID) api method. Changed: - Appended field 'string withdrawal_tx_id = 40' to pb.proto's Trade message. - Added nullable 'String withdrawalTxId' to Trade entity class. - Added trade.setWithdrawalTxId(transaction.getTxId().toString()) in TradeManager#onWithdrawRequest's callback.
|
Force travis jar download |
As per commit 88f26f9, do not autofill all currencies by default but keep all unselected.
This change was prompted by the recent changes in the main branch to allow a tx memo field to be set from the UI and API. This and the prior PR address the API's need to be able to fetch a tx (with a memo). The API can now get a completed trade's withdrawal txid and pass it as a gettransaction parameter. See previous PR "Append nullable withdrawalTxId field to Trade". bisq-network#4937 A summary of changes by file: grpc.proto - Added withdrawalTxId field to existing TradeInfo proto & wrapper. - Reordered fields in TradeInfo proto. - Added new fields to be displayed by TxInfo proto in CLI. - Fixed typo: unsetTxFeeRatePreference -> UnsetTxFeeRatePreference. - Added new GetTransaction rpc. GrpcWalletsService - Added new getTransaction gRPC boilerplate. CoreWalletsService - Added new getTransaction implementation. TxInfo - Added the new fields for displaying a tx summary from CLI. This is not intended to be more than a brief summary; a block explorer or bitcoin-core client should be used to see the complete definition. TradeInfo - Added the new withdrawalTxId field defined in grpc.proto. CliMain - Added new 'case gettransaction'. TransactionFormat - Formats a TxInfo sent from the server to CLI. ColumnHeaderConstants - Added console headers used by TransactionFormat. TradeFormat - Displays a completed trade's WithdrawalTxId if present. Apitest - Adjusted affected tests: assert tx memo is persisted and test gettransaction.
| @Nullable | ||
| @Getter | ||
| @Setter | ||
| private String withdrawalTxId; |
There was a problem hiding this comment.
I'm not convinced about adding this tx to the trade. The Trade object is rather bloated anyway and I don't feel this tx is really associated.
There was a problem hiding this comment.
Your call. I'll adjust the next PR if you don't want it.
I thought it was an inexpensive way to associate the withdrawal tx to the trade. To me, setting a memo on a trade withdrawal tx isn't very useful if you don't have an easy way to find that tx.
By the way, this tx is associated (in my head) when I optionally withdraw (seller) trade funds to an external wallet, as the final step of the trade protocol.
There was a problem hiding this comment.
Perhaps you're right. I'm just cautious of adding more stuff to Trade. Will have a sleep on it.
There was a problem hiding this comment.
I did not think this would make much sense to anyone until they've seen the next PR, and I explain my reason a bit more/better.
Here is Bob's gettrade CLI output after withdrawing funds to an external wallet:
ID My Role Price in USD for 1 BTC Amount(BTC) Tx Fee(BTC) Taker Fee(BTC) Deposit Published Deposit Confirmed Fiat Sent Fiat Received Payout Published Withdrawn Withdrawal TX ID
5006844 BTC buyer as taker 19,410.6419 0.12500000 0.00012932 0.00037500 YES YES YES YES YES YES 0128e08406542a15ecc27234afa9ca9f4112f74d105459d18b4cfde93028291b
If Withdrawn = NO, the Withdrawal TX ID column would not appear in the console output.
Here is Bob's gettransaction CLI output:
Tx ID Is Confirmed Tx Inputs (BTC) Tx Outputs (BTC) Tx Fee (BTC) Tx Size (Bytes) Memo
0128e08406542a15ecc27234afa9ca9f4112f74d105459d18b4cfde93028291b YES 0.14375000 0.14368229 0.00006771 193 Bob's trade withdrawal
Likewise, if there is no memo, the Memo column is not displayed.
There was a problem hiding this comment.
The payout address might be useful for looking up the spent tx of that utxo which is the withdrawal tx.
I am not so sure if we should add that to the trade as it is not really part of the trade. The trade ends when you have received the funds. Most users I assume keep the funds anyway in the wallet so there is no withdrawal tx but the funds are used as input for another offer or trade.
There was a problem hiding this comment.
Thought so to begin with as well. Now I'm thinking it makes more sense to associate the withdrawal with the trade as it's a way to know where the bitcoin went. Considering we have the feature to fund trades externally and withdraw directly it seems this withdrawal is directly related to this trade.
I now have an utACK for this PR.
There was a problem hiding this comment.
NACK
As discussed in keybase I think the withdrawal is not part of the trade conceptually. If funds are kept in Bisq the funds are likely used to fund the next trade fee tx and that is tagged with the new offer/trade, so then the same tx would appear in 2 different trades.
|
Closing, and reverting this PR's commit in #4948. |
The
withdrawalTxIdfield will be set inTradeManager#onWithdrawRequestafter successful trade completion and withdrawal of funds.Persisting
withdrawalTxIdwill allow the api and ui to find thewithdrawalTxIdfor a completed trade after the seller withdraws funds to an external wallet. In turn, the withdrawal tx's memo field will be accessible in a new apigetTransaction(id)api method (todo) .Changed:
Appended field
string withdrawal_tx_id = 40to pb.proto's Trade message.Added nullable
String withdrawalTxIdto Trade entity class.Added
trade.setWithdrawalTxId(transaction.getTxId().toString())inTradeManager#onWithdrawRequest's callback.This is the 7th in a chain of PRs beginning with #4884.
PR #4936 should be reviewed before this one.