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.

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

API daemon 'getoffer' & 'gettrade' support for BSQ swaps #1 #5827

Merged
merged 14 commits into from
Dec 14, 2021

Conversation

ghubstan
Copy link
Member

This change is a partial adjustment in the API for BSQ swaps, mostly related to how it fetches and displays v1 and new swap offers and trades.

  • The currencyCode param was removed from the GetBsqSwapOffer service request.

  • BsqSwapOfferInfo fields were merged into OfferInfo, and BsqSwapOfferInfo was removed.

  • New rpc service GetOfferCategory was added for later use, to determine what kind of rpc request to send in CLI takeoffer calls (v1-offer or bsq-swap offer).

  • Added new coreApi.getRole(BsqSwapTrade) method.

  • Normalized (removed duplication) recently merged BsqSwapTradeInfo with the existing TradeInfo gRPC proto defs and wrappers. A bsqSwapTradeInfo field was appended to the old TradeInfo proto message. The API's gettrade and gettrades (todo) methods' reply messages can still return the same gRPC response message data type (TradeInfo, or List<TradeInfo> for gettrades), and may represent v1 trade and/or bisq-swap trade details.

  • Adjusted the CLI console output to display v1 or bsq-swap trade details returned by the CLI's gettrade method.

BSQ swap offer create & take support in the CLI should come with the next 1-3 PRs.

Based on master.

- Adjust GetBsqSwapOffer(s) rpc services to remove currency param.
- Adjust OfferInfo and remove BsqSwapOfferInfo.
- Add GetOfferCategory service so CLI can determine what kind of takeoffer service is to be used.
- Add comment about adding sub-message BsqSwapTradeInfo field to TradeInfo.
- Add GetOfferCategory service so CLI can determine what kind of takeoffer service is to be used.
- Adjust to removal of BsqSwapOfferInfo proto.
- Call new coreApi.getRole(BsqSwapTrade) before building BsqSwapTradeInfo proto.
- Complete BsqSwapTradeInfo impl.
- Merge BsqSwapOfferInfo fields into OfferInfo and remove BsqSwapOfferInfo.
- Change all model builders to private static class Builder.
- Add core api methods to help CLI determine which type of offer to
  take for a given offerId.  CLI's 'takeoffer` will need to determine
  which gRPC/proto request type to send to server.

- Add implemetations for getBsqSwapTradeRole(), for tradeId or trade.
- Adjust to removal of BsqSwapOfferInfo proto, use ammended OfferInfo instead.

- Remove currency-code param from all get(My)BsqSwapOffer(s) requests.

- Add new OfferCategory getAvailableOfferCategory(String offerId) service.
  CLI uses this to determine which kind of gRPC request object should be
  sent with a 'takeoffer' request.

- Adjust GetOffersSmokeTest to help see offer/swap-offer CLI output.
- Adjust to removal of BsqSwapOfferInfo proto, use ammended OfferInfo instead.
- Remove currency-code param from all get(My)BsqSwapOffer(s) requests.
This commit refactors the first cut of the BsqSwapTradeInfo and
TradeInfo gRPC proto defs and wrappers.  The change avoids duplication
of fields between BsqSwapTradeInfo and TradeInfo, and adds a
bsqSwapTradeInfo field to the old TradeInfo proto & wrapper.

The immediate goal is moving towards getting the API's 'gettrade'
method to work for both Bisq v1 trades and BSQ swap trades:  the TradeInfo
proto sent to the CLI should represent either a Bisq v1 trade or a BSQ
swap trade.  A mid-term term goal is to also make a new 'gettrades' method
return a List<TradeInfo> to the CLI, where items in the List<TradeInfo>
can be either v1 trades or bsq-swap trades.
- Made several adjustments to CLI's 'gettrade' output related code
  so it can show single trade details for either Bisq v1 trades, or
  BSQ swap trades.

- Did minor refactoring of API's core to retrieve # tx confirmations
  for an addresses and transactions.

- Show # of tx confirmations in bsq swap trade detail.
@ripcurlx ripcurlx added this to the v1.8.0 milestone Nov 16, 2021
@ripcurlx
Copy link
Contributor

I tried getoffer, getmyoffer, getoffers, getmyoffers, editoffer and gettrade. editoffer fails as it is not implemented I guess, but gettrade also fails for bsq swap trades. Is this not to be expected to work in this PR as well?

./bisq-cli --password=mySecret gettrade --trade-id=411395-b5a74a47-ecfd-4111-9199-1c060261b281-175
ID      My Role              Price in BTC for 1 BSQ  Amount(BSQ)  Tx Fee (BTC)  Maker Fee(BSQ)  Deposit Published  Deposit Confirmed  Buyer Cost(BTC)  BSQ Sent  BSQ Received  Payout Published  Withdrawn  BSQ Buyer Address
411395  BSQ seller as maker              0.00004000       250.00    0.00002450            0.00  YES                NO                      0.01000000  NO        NO            NO                NO         Bbcrt1qu5tcy8kjmp09jdur78rq0djjar9hdla24rtlj5
./bisq-cli --password=mySecret gettrade --trade-id=9510828-952a1e3c-d658-4d9b-91ec-da75b830912e-175
Error: trade with id '9510828-952a1e3c-d658-4d9b-91ec-da75b830912e-175' not found

@ghubstan
Copy link
Member Author

I tried getoffer, getmyoffer, getoffers, getmyoffers, editoffer and gettrade. editoffer fails as it is not implemented I guess, but gettrade also fails for bsq swap trades. Is this not to be expected to work in this PR as well?

I have not addressed the CLI side of this, and do not expect it to work. (The PR title should have specified these are incremental changes, mostly on the server side.) This will require 2-3 more PRs.

I am a bit surprised that gettrade did not work, but again, I have not worked on the CLI side of gettrade.

Please un-comment the @Disabled annotation in bisq.apitest.method.trade.BsqSwapTradeTest, and run it with the -DrunApiTests=true jvm-opt? That should work because of the changes to the related gRPC/protobuf defs and classes, and other changes on the 'server' side. To see what gettrade will show (in future PR), set <root level="DEBUG"> in the apitest/test/resources/logback.xml.

@ghubstan ghubstan changed the title API 'getoffer' & 'gettrade' support for BSQ swaps API 'getoffer' & 'gettrade' support for BSQ swaps (but not @ CLI level) Nov 23, 2021
@ghubstan ghubstan changed the title API 'getoffer' & 'gettrade' support for BSQ swaps (but not @ CLI level) [WIP] API 'getoffer' & 'gettrade' support for BSQ swaps (but not @ CLI level) Nov 23, 2021
@ghubstan ghubstan marked this pull request as draft November 23, 2021 22:27
@ghubstan ghubstan changed the title [WIP] API 'getoffer' & 'gettrade' support for BSQ swaps (but not @ CLI level) [WIP] API daemon 'getoffer' & 'gettrade' support for BSQ swaps #1 Nov 24, 2021
@ghubstan ghubstan changed the title [WIP] API daemon 'getoffer' & 'gettrade' support for BSQ swaps #1 API daemon 'getoffer' & 'gettrade' support for BSQ swaps #1 Nov 24, 2021
@ghubstan ghubstan marked this pull request as ready for review November 24, 2021 16:57
@ripcurlx ripcurlx modified the milestones: v1.8.0, v1.8.1 Dec 9, 2021
sqrrm
sqrrm previously approved these changes Dec 9, 2021
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

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

utACK

@ghubstan I haven't followed other developments, can this be merged or does it need to wait?

Comment on lines +495 to +497
public static boolean isAltcoinOffer(Offer offer) {
return offer.getCounterCurrencyCode().equals("BTC") && !offer.isBsqSwapOffer();
}
Copy link
Member

Choose a reason for hiding this comment

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

Is swapOffer not considered an altcoin trade? I guess it makes some sense but also somewhat odd.

@sqrrm sqrrm merged commit b65cc9c into bisq-network:master Dec 14, 2021
@ghubstan ghubstan deleted the 1-basic-api-bsqswap-support branch January 3, 2022 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants