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.

Sign up for GitHub

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

Append nullable withdrawalTxId field to Trade proto message #4937

Closed
wants to merge 13 commits into from
Closed

Append nullable withdrawalTxId field to Trade proto message #4937

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
dc6144d
Refactor BtcWalletService to let api override fee rates
ghubstan Dec 4, 2020
900d498
Merge branch 'master' into 02-refactor-completePreparedSendBsqTx
ghubstan Dec 4, 2020
159d4cc
Add optional txFeeRate parameter to api sendbsq
ghubstan Dec 4, 2020
2842070
Merge branch 'master' into 03-add-txFeeRate-param
ghubstan Dec 8, 2020
6c9f0c2
Add new api method 'sendbtc' and test
ghubstan Dec 9, 2020
144c5a8
Merge branch 'master' into 04-add-sendbtc-impl
ghubstan Dec 9, 2020
bd66008
Support tx memo field for btc withdrawals from api
ghubstan Dec 9, 2020
478c8f4
Remove unused imports
ghubstan Dec 9, 2020
259bad6
Merge branch 'master' into 05-use-memo-tx-field
ghubstan Dec 10, 2020
150e2f6
Use Bisq's UserThread.executor in gRPC server
ghubstan Dec 11, 2020
6aa385e
Append nullable withdrawalTxId field to Trade proto message
ghubstan Dec 12, 2020
34a4b74
Merge branch 'master' into 07-trade-withdrawaltxid
ghubstan Dec 14, 2020
4a09275
Adjust create TransferwiseAccount test
ghubstan Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions core/src/main/java/bisq/core/trade/Trade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,10 @@ public static protobuf.Trade.TradePeriodState toProtoMessage(Trade.TradePeriodSt
@Getter
@Setter
private String payoutTxId;
@Nullable
@Getter
@Setter
private String withdrawalTxId;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you're right. I'm just cautious of adding more stuff to Trade. Will have a sleep on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@Getter
@Setter
private long tradeAmountAsLong;
Expand Down Expand Up @@ -554,6 +558,7 @@ public Message toProtoMessage() {
Optional.ofNullable(takerFeeTxId).ifPresent(builder::setTakerFeeTxId);
Optional.ofNullable(depositTxId).ifPresent(builder::setDepositTxId);
Optional.ofNullable(payoutTxId).ifPresent(builder::setPayoutTxId);
Optional.ofNullable(withdrawalTxId).ifPresent(builder::setWithdrawalTxId);
Optional.ofNullable(tradingPeerNodeAddress).ifPresent(e -> builder.setTradingPeerNodeAddress(tradingPeerNodeAddress.toProtoMessage()));
Optional.ofNullable(contract).ifPresent(e -> builder.setContract(contract.toProtoMessage()));
Optional.ofNullable(contractAsJson).ifPresent(builder::setContractAsJson);
Expand Down Expand Up @@ -587,6 +592,7 @@ public static Trade fromProto(Trade trade, protobuf.Trade proto, CoreProtoResolv
trade.setTakerFeeTxId(ProtoUtil.stringOrNullFromProto(proto.getTakerFeeTxId()));
trade.setDepositTxId(ProtoUtil.stringOrNullFromProto(proto.getDepositTxId()));
trade.setPayoutTxId(ProtoUtil.stringOrNullFromProto(proto.getPayoutTxId()));
trade.setWithdrawalTxId(ProtoUtil.stringOrNullFromProto(proto.getWithdrawalTxId()));
trade.setContract(proto.hasContract() ? Contract.fromProto(proto.getContract(), coreProtoResolver) : null);
trade.setContractAsJson(ProtoUtil.stringOrNullFromProto(proto.getContractAsJson()));
trade.setContractHash(ProtoUtil.byteArrayOrNullFromProto(proto.getContractHash()));
Expand Down Expand Up @@ -1124,6 +1130,7 @@ public String toString() {
",\n takerFeeTxId='" + takerFeeTxId + '\'' +
",\n depositTxId='" + depositTxId + '\'' +
",\n payoutTxId='" + payoutTxId + '\'' +
",\n withdrawalTxId='" + withdrawalTxId + '\'' +
",\n tradeAmountAsLong=" + tradeAmountAsLong +
",\n tradePrice=" + tradePrice +
",\n tradingPeerNodeAddress=" + tradingPeerNodeAddress +
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/bisq/core/trade/TradeManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,7 @@ public void onSuccess(@javax.annotation.Nullable Transaction transaction) {
onTradeCompleted(trade);
trade.setState(Trade.State.WITHDRAW_COMPLETED);
getTradeProtocol(trade).onWithdrawCompleted();
trade.setWithdrawalTxId(transaction.getTxId().toString());
requestPersistence();
resultHandler.handleResult();
}
Expand Down
1 change: 1 addition & 0 deletions proto/src/main/proto/pb.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1450,6 +1450,7 @@ message Trade {
string counter_currency_extra_data = 37;
string asset_tx_proof_result = 38; // name of AssetTxProofResult enum
string uid = 39;
string withdrawal_tx_id = 40;
}

message BuyerAsMakerTrade {
Expand Down