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 BSQ swap simulation script and doc updates #5 #5876

Merged

Conversation

ghubstan
Copy link
Member

Based on PR #5874

- 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.
The old GetTrade and TakeOffer rpc service defs will be used
for getting BSQ swap trades, and taking BSQ swap offers.
… swaps

The rpc GetBsqSwapTrade and TakeBsqSwapOffer services were a quick hack
to help test BSQ swap feature development more quickly, using apitest
cases.  Their gRPC service method implementations are removed here and
the GetTrade & TakeOffer service methods are refactored to support BSQ
swaps.
A minor refactoring to support serving TradeModel instances to CLI.
For example, the CLI 'gettrade' command must return a v1 Trade or
BSQ swap trade.
Also refactored some of the opt parsers.
There are some use cases where the CLI needs to know what kind of offer
is being acted on before the request is made, For example:

There are differences between a BsqSwap 'takeoffer'request, and a v1
'takeoffer' request.

A BsqSwap offer cannot be edited by an 'editoffer' request, and an
attempt should be blocked by the CLI.

- Append isMyOffer GetOfferCategoryRequest rpc msg def.

- Adjust daemon.grpc services for new boolean GetOfferCategoryRequest param.

- Adjust core.api for new boolean GetOfferCategoryRequest param.

- Add validation check in core.api EditOfferValidator to block attempt to
  edit a BsqSwap offer.

- Refactor CoreOffersService get*offer(id) methods to optionally throw
  excpetions.
@ghubstan ghubstan changed the title [WIP] API BSQ swap simulation script and doc updates #5 API BSQ swap simulation script and doc updates #5 Nov 29, 2021
@ghubstan ghubstan marked this pull request as ready for review November 29, 2021 14:31
The OfferIdOptionParser superclass reduces duplication for parsing
offer-id parameters, but it needs to let subclass parsers' other
options pass validation.
Plus some outdated JDK version compat comments.
@ripcurlx
Copy link
Contributor

ripcurlx commented Dec 3, 2021

@ghubstan Just a heads up that I'm starting to review this chain of PRs now.

@ripcurlx
Copy link
Contributor

ripcurlx commented Dec 3, 2021

I'm just reading through the documentation and I think it might be good to keep the option open that we'll support multiple coin swaps via the API as well. I know you asked me this before, so sorry for not replying on this in-time.

Please ignore this comment.

So instead of

$ ./bisq-cli --password=xyz --port=9998 createoffer \
    --swap=true \
    --direction=BUY \
    --amount=0.5 \
    --fixed-price=0.00005

maybe

$ ./bisq-cli --password=xyz --port=9998 createoffer \
    --swap=true \
    --currency-code=BSQ
    --direction=BUY \
    --amount=0.5 \
    --fixed-price=0.00005

WDYT?

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK

Hi @ghubstan! I walked through the documentation as an API consumer and tested following API endpoints. You find the complete walkthrough with outputs below. Great work so far! Besides the getoffer problem, just a couple of minor things that needs to be fixed from a consumer perspective. I'll do the code review after those parts are fixed. Thanks!

Summary

  • createoffer (documentation is wrong) ✔️
  • getoffers
  • getmyoffers
  • getoffer (not working for BSQ swap) ❌
  • editoffer (error message could be improved) ✔️
  • canceloffer
  • gettrade
  • takeoffer

I've tested as a API consumer getoffers and getmyoffers which work as expected.
When testing the example form the docs to create a BSQ offer:

./bisq-cli --password=xyz --port=9998 createoffer \
    --swap=true \
    --direction=BUY \
    --amount=0.5 \
    --fixed-price=0.00005

it fails with Error: no currency code specified

./bisq-cli --password=xyz --port=9998 createoffer \
    --currency-code=BSQ \
    --swap=true \
    --direction=BUY \
    --amount=0.5 \
    --fixed-price=0.00005

works just fine resulting in following output:

Enabled  Buy/Sell            Price in BTC for 1 BSQ  BTC(min - max)  BSQ(min - max)  Payment Method  Creation Date (UTC)   ID
PENDING  Sell BSQ (Buy BTC)              0.00005000      0.50000000       10,000.00  BSQ Swap        2021-12-03T13:33:02Z  1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175

After a couple of seconds I tried to request the offer with:

./bisq-cli --password=xyz getoffer --offer-id=1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175

which failed with

Error: offer with id '1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175' not found

Requesting all offers with:

./bisq-cli --password=xyz getmyoffers --direction=BUY --currency-code=BSQ                     

did succeed.

Enabled  Buy/Sell            Price in BTC for 1 BSQ  BTC(min - max)  BSQ(min - max)  Payment Method  Creation Date (UTC)   ID
YES      Sell BSQ (Buy BTC)              0.00005000      0.50000000       10,000.00  BSQ Swap        2021-12-03T13:33:02Z  1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175 
YES      Sell BSQ (Buy BTC)              0.00004000      0.01000000          250.00  Altcoins        2021-12-03T13:25:55Z  mHRQoPHK-1ece3745-d94a-45e8-bd5c-6ba81bf9363b-175
YES      Sell BSQ (Buy BTC)              0.00003900      0.01200000          307.69  BSQ Swap        2021-12-03T13:26:14Z  RQAVAU8-430bbe62-b286-4277-8bd4-8c1c1db15fbe-175 

Testing editing of a BSQ offer with:

./bisq-cli --password=xyz editoffer --offer-id=1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175 --fixed-price=0.00004000 --enable=false

Fails with

Error: cannot edit swap bsq offers

Which is fine as editing of BSQ offers (Should be BSQ instead of bsq IMO) isn't necessary as you can remove and create them without paying trading or mining fees. Still I think it would be good to hint the user in the error message why and what to do instead.

So I tried to cancel and create the offer instead:

./bisq-cli --password=xyz canceloffer --offer-id=1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175
offer canceled and removed from offer book
./bisq-cli --password=xyz createoffer \                                                          
    --currency-code=BSQ \
    --swap=true \
    --direction=BUY \
    --amount=0.5 \
    --fixed-price=0.000065
Enabled  Buy/Sell            Price in BTC for 1 BSQ  BTC(min - max)  BSQ(min - max)  Payment Method  Creation Date (UTC)   ID
PENDING  Sell BSQ (Buy BTC)              0.00006500      0.50000000        7,692.31  BSQ Swap        2021-12-03T13:49:23Z  14286-a32584e1-1591-4804-bcb5-acb342d17e6c-175

which worked just fine.

Now I took the offer with a non-API client. Checking the state with getmyoffers.

./bisq-cli --password=xyz getmyoffers --direction=BUY --currency-code=BSQ
Enabled  Buy/Sell            Price in BTC for 1 BSQ  BTC(min - max)  BSQ(min - max)  Payment Method  Creation Date (UTC)   ID
YES      Sell BSQ (Buy BTC)              0.00004000      0.01000000          250.00  Altcoins        2021-12-03T13:25:55Z  mHRQoPHK-1ece3745-d94a-45e8-bd5c-6ba81bf9363b-175
YES      Sell BSQ (Buy BTC)              0.00003900      0.01200000          307.69  BSQ Swap        2021-12-03T13:26:14Z  RQAVAU8-430bbe62-b286-4277-8bd4-8c1c1db15fbe-175

As expected the offer is gone and can be found as an active trade:

./bisq-cli --password=xyz gettrade --trade-id=14286-a32584e1-1591-4804-bcb5-acb342d17e6c-175
ID     My BSQ Swap Role     Price in BTC for 1 BSQ  Amount(BSQ)  Tx Fee (BTC)  Maker Fee(BSQ)  Buyer Cost(BTC)  Status     Tx ID                                                             Confirmations
14286  BSQ seller as maker              0.00006500     7,692.31    0.00000000            0.25       0.50000000  COMPLETED  361e973033e7852e9347d5eff108da58d461a89e56015170865214f15ddcdcda              0

After the first block confirmation it looks like:

./bisq-cli --password=xyz gettrade --trade-id=14286-a32584e1-1591-4804-bcb5-acb342d17e6c-175
ID     My BSQ Swap Role     Price in BTC for 1 BSQ  Amount(BSQ)  Tx Fee (BTC)  Maker Fee(BSQ)  Buyer Cost(BTC)  Status     Tx ID                                                             Confirmations
14286  BSQ seller as maker              0.00006500     7,692.31    0.00000000            0.25       0.50000000  COMPLETED  361e973033e7852e9347d5eff108da58d461a89e56015170865214f15ddcdcda              1

Next I tried to take a BSQ swap offer of a non-API client.

./bisq-cli --password=xyz getoffers --direction=BUY --currency-code=BSQ                       
Buy/Sell            Price in BTC for 1 BSQ  BTC(min - max)  BSQ(min - max)  Payment Method  Creation Date (UTC)   ID
Sell BSQ (Buy BTC)              0.00004000      0.10000000        2,500.00  BSQ Swap        2021-11-24T09:57:35Z  87384764-a4842168-b7a7-4a95-8a0f-34982108b58d-180
Sell BSQ (Buy BTC)              0.00004000      0.00100000           25.00  Altcoins        2021-11-25T10:30:41Z  tuvkkg-319f04dc-1b17-4e72-8a71-e30cb536bce9-180  
./bisq-cli --password=xyz --port=9998 takeoffer --offer-id=87384764-a4842168-b7a7-4a95-8a0f-34982108b58d-180
trade 87384764-a4842168-b7a7-4a95-8a0f-34982108b58d-180 successfully taken
./bisq-cli --password=xyz gettrade --trade-id=87384764-a4842168-b7a7-4a95-8a0f-34982108b58d-180
ID        My BSQ Swap Role    Price in BTC for 1 BSQ  Amount(BSQ)  Tx Fee (BTC)  Taker Fee(BSQ)  Buyer Cost(BTC)  Status     Tx ID                                                             Confirmations
87384764  BSQ buyer as taker              0.00004000     2,500.00    0.00001320            0.30       0.10000000  COMPLETED  fd4495141dfb9eb614e98ddb18081edbafafd474f31f9603d6d760ce440cc60b              0
./bisq-cli --password=xyz gettrade --trade-id=87384764-a4842168-b7a7-4a95-8a0f-34982108b58d-180
ID        My BSQ Swap Role    Price in BTC for 1 BSQ  Amount(BSQ)  Tx Fee (BTC)  Taker Fee(BSQ)  Buyer Cost(BTC)  Status     Tx ID                                                             Confirmations
87384764  BSQ buyer as taker              0.00004000     2,500.00    0.00001320            0.30       0.10000000  COMPLETED  fd4495141dfb9eb614e98ddb18081edbafafd474f31f9603d6d760ce440cc60b              1

@ghubstan
Copy link
Member Author

ghubstan commented Dec 3, 2021

After a couple of seconds I tried to request [MY] offer with:

./bisq-cli --password=xyz getoffer --offer-id=1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175

which failed with

Error: offer with id '1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175' not found

Requesting all offers with:

./bisq-cli --password=xyz getmyoffers --direction=BUY --currency-code=BSQ                     

did succeed.

If you want to request your own offer, you need to use the getmyoffer command, not getoffer command, which looks for available offers created by other users.

This should have worked, and did for me while trying to reproduce the problem:
./bisq-cli --password=xyz getmyoffer --offer-id=1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175

And you saw that requesting my offers (plural) did work as expected:
./bisq-cli --password=xyz getmyoffers --direction=BUY --currency-code=BSQ
Calling getoffers (available offers) would not have worked.

@ghubstan
Copy link
Member Author

ghubstan commented Dec 3, 2021

As expected the offer is gone and can be found as an active trade:

Did you mean the offer you canceled is gone, but the offer you did not cancel can be found as an active trade? And that is correct after all?

@ghubstan
Copy link
Member Author

ghubstan commented Dec 3, 2021

Which is fine as editing of BSQ offers (Should be BSQ instead of bsq IMO) isn't necessary as you can remove and create them without paying trading or mining fees. Still I think it would be good to hint the user in the error message why and what to do instead.

How about: bsq swap offers cannot be edited, but you may cancel them without forfeiting any funds ?
Commit f195b76

RE: Should be BSQ instead of bsq IMO
For consistency, all the CLI error msgs are lower case, including any currency codes. You think it's OK to leave it as it is in this PR, and address this issue in a future PR dedicated to reviewing all, and changing some CLI error msgs?

@ripcurlx
Copy link
Contributor

ripcurlx commented Dec 4, 2021

After a couple of seconds I tried to request [MY] offer with:

./bisq-cli --password=xyz getoffer --offer-id=1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175

which failed with

Error: offer with id '1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175' not found

Requesting all offers with:

./bisq-cli --password=xyz getmyoffers --direction=BUY --currency-code=BSQ                     

did succeed.

If you want to request your own offer, you need to use the getmyoffer command, not getoffer command, which looks for available offers created by other users.

This should have worked, and did for me while trying to reproduce the problem: ./bisq-cli --password=xyz getmyoffer --offer-id=1376837-1edf81cf-49bb-4a88-97cd-143bcf850025-175

And you saw that requesting my offers (plural) did work as expected: ./bisq-cli --password=xyz getmyoffers --direction=BUY --currency-code=BSQ Calling getoffers (available offers) would not have worked.

I forgot about our separation of getoffers, getoffer and getmyoffers, getmyoffer. Still I'm not sure if we really need the getmyoffer endpoint and not just use the getoffer endpoint. As you pass the offer id I would always expect to get the offer no matter if it was my offer or someone else offer. There is no auto filtering taking place as for all the other methods. WDYT?

@ghubstan
Copy link
Member Author

ghubstan commented Dec 4, 2021

I'm not sure if we really need the getmyoffer endpoint and not just use the getoffer endpoint. As you pass the offer id I would always expect to get the offer no matter if it was my offer or someone else offer. There is no auto filtering taking place as for all the other methods. WDYT?

I agree. Is it OK if I make the change

  • deprecate getmyoffer
  • make getoffer return the offer regardless of ownership

in a new PR, on top of the last & next PR in this chain, ending in #5893?
The change request seems to deserve its own PR, and that would let me work on top of the #5893 changes for XMR support.

And finally, to be clear about the scope of the requested change: API endpoints getmyoffers and getoffers will remain as is. Is that right?

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

cli/src/main/java/bisq/cli/CliMain.java Outdated Show resolved Hide resolved
@sqrrm sqrrm merged commit cf81fd4 into bisq-network:master Dec 14, 2021
@ghubstan ghubstan deleted the 5-api-bsqswap-simulation-n-docs-update branch January 3, 2022 12:34
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