API XMR/BTC trading pair support (#6) #5893

ghubstan · 2021-12-03T16:22:12Z

New API features:

Create XMR payment accts with API.
Create XMR offers with API.
Trade XMR using v1 protocol, not swapping.

Testing new API XMR support, docs:

Created apitest cases for XMR support.
Created XMR/BTC trade simulation script (regtest only).
Updated api beta test guide for XMR support.

Based on PR #5876

V1 offer instances have a makeFee value, but not BSQ swap offers. This needs to be investigated. See commit d57d6e9.

This makes it easier to compare to EditOfferValidator.toString

The editoffer validation bug fixes: - A trigger-price edit forced offer.price-margin=0.00. This needs to be checked in new apitest case asserts. - An activate state (only) edit forced offer.isUseMarketBasedPrice=true. The CLI does not have the offer instance, and cannot know the correct value of the isUseMarketBasedPrice param sent in the editoffer request. The daemon has to figure this out. If the editType parameter value sent to daemon is ACTIVATION_STATE_ONLY, use the current offer.isUseMarketBasedPrice. The refactoring includes more useful and readable information in core's EditOfferValidator and MutableOfferPayloadFields toString methods, for debugging with the daemon log. And some adjustments for allowing edits to XMR offers.

… support

There are no asserts, but helpful for looking at affects of CLI commands from terminal because apitest cases use gRPC service stubs instead of terminal.

Method tests are combined in scenario pkg tests to reduce test harness startup/shutdown time.

…trading

ripcurlx · 2021-12-20T11:41:49Z

I just went through the scenarios of creating an XMR account and executed a successful trade. Here are my comments as an API user.

createcryptopaymentacct: ☑️
I think we should add this to the documentation as we do it for createpaymentacct as well. ATM you need to look at simulation scripts to find out how to create altcoin accounts which might be the main use cases for API usage. Maybe we could even just re-use the createpaymentacct method with different parameters to make altcoin accounts a first class citizen.
createoffer: ☑️
Is there a reason why we named the payment account id argument --payment-account and not --payment-account-id? I think it would be more consistent with the other API methods if we would support the argument with ...-id as well. WDYT?
editoffer ✅

Trading process

Everything worked as expected I only recognized this time that using the keepfunds command is necessary to complete the trade (or isn't it?), but when just checking the gettrade command there is no change happening. So this might be confusing for API users.

➜  bisq git:(6-api-xmr-trading) ✗ ./bisq-cli --password=mySecret gettrade --trade-id=yfxmel-4f5caead-1ceb-459e-8111-bc5b2f259e42-180
ID      My Role              Price in BTC for 1 XMR  Amount(XMR)  Tx Fee (BTC)  Maker Fee(BSQ)  Deposit Published  Deposit Confirmed  Buyer Cost(BTC)  XMR Sent  XMR Received  Payout Published  Withdrawn  XMR Buyer Address
yfxmel  XMR seller as maker              0.00395772         5.05    0.00002450            0.03  YES                YES                     0.02000000  YES       YES           YES               NO         46tM15KsogEW5MiVmBn7waPF8u8ZsB6aHjJk7BAv1wvMKfWhQ2h2so5BCJ9cRakfPt5BFo452oy3K8UK6L2u2v7aJ3Nf7P2
➜  bisq git:(6-api-xmr-trading) ✗ ./bisq-cli --password=mySecret --port=9998 keepfunds --trade-id=yfxmel-4f5caead-1ceb-459e-8111-bc5b2f259e42-180
funds from trade yfxmel-4f5caead-1ceb-459e-8111-bc5b2f259e42-180 saved in bisq wallet
➜  bisq git:(6-api-xmr-trading) ✗ ./bisq-cli --password=mySecret gettrade --trade-id=yfxmel-4f5caead-1ceb-459e-8111-bc5b2f259e42-180             
ID      My Role              Price in BTC for 1 XMR  Amount(XMR)  Tx Fee (BTC)  Maker Fee(BSQ)  Deposit Published  Deposit Confirmed  Buyer Cost(BTC)  XMR Sent  XMR Received  Payout Published  Withdrawn  XMR Buyer Address
yfxmel  XMR seller as maker              0.00395772         5.05    0.00002450            0.03  YES                YES                     0.02000000  YES       YES           YES               NO         46tM15KsogEW5MiVmBn7waPF8u8ZsB6aHjJk7BAv1wvMKfWhQ2h2so5BCJ9cRakfPt5BFo452oy3K8UK6L2u2v7aJ3Nf7P2

Maybe adding another column with trade state?

ghubstan · 2021-12-21T21:32:41Z

Is there a reason why we named the payment account id argument --payment-account and not --payment-account-id?
I think it would be more consistent with the other API methods if we would support the argument with ...-id as well. WDYT?

At the time I thought abbreviating the param name was a good idea, but it was not. I should make it consistent and change it to --payment-account-id.

ghubstan · 2021-12-21T21:39:35Z

Everything worked as expected I only recognized this time that using the keepfunds command is necessary to complete the
trade (or isn't it?), but when just checking the gettrade command there is no change happening.
So this might be confusing for API users.

This is confusing and seemingly unnecessary to API users, but if it is not done, the completed trade will not be moved from the pending/open trades list (persistence file), to the closed trades list (persistence file). I am almost certain there is a keep funds button click required as the last step in the UI / protocol. (The alternative last step in the UI is to move-funds-to-external-wallet.)

The keepfunds cmd is consistent with the final step in the UI. I think this needs some more study from both of us before I change it to say... automatically moving completed trade from open-trades list to closed-trades list in the "next to last" step of the protocol.

ripcurlx · 2021-12-22T08:51:41Z

createcryptopaymentacct: ☑️
I think we should add this to the documentation as we do it for createpaymentacct as well. ATM you need to look at simulation scripts to find out how to create altcoin accounts which might be the main use cases for API usage. Maybe we could even just re-use the createpaymentacct method with different parameters to make altcoin accounts a first class citizen.

@ghubstan Any thoughts on this as well?

ghubstan · 2021-12-22T11:12:12Z

Any thoughts on this as well?

Not yet. I hope to have some thought-time later today.

ghubstan · 2021-12-22T21:48:47Z

Is there a reason why we named the payment account id argument --payment-account and not --payment-account-id?
I think it would be more consistent with the other API methods if we would support the argument with ...-id as well. WDYT?

At the time I thought abbreviating the param name was a good idea, but it was not. I should make it consistent and change it to --payment-account-id.

@ripcurlx, do you agree a resolution of this issue deserves its own PR?

ghubstan · 2021-12-22T21:50:09Z

Everything worked as expected I only recognized this time that using the keepfunds command is necessary to complete the
trade (or isn't it?), but when just checking the gettrade command there is no change happening.
So this might be confusing for API users.

This is confusing and seemingly unnecessary to API users, but if it is not done, the completed trade will not be moved from the pending/open trades list (persistence file), to the closed trades list (persistence file). I am almost certain there is a keep funds button click required as the last step in the UI / protocol. (The alternative last step in the UI is to move-funds-to-external-wallet.)

The keepfunds cmd is consistent with the final step in the UI. I think this needs some more study from both of us before I change it to say... automatically moving completed trade from open-trades list to closed-trades list in the "next to last" step of the protocol.

@ripcurlx, do you agree the resolution of this issue deserves its own PR?

ghubstan · 2021-12-22T21:52:13Z

createcryptopaymentacct: ballot_box_with_check
I think we should add this to the documentation as we do it for createpaymentacct as well. ATM you need to look at simulation scripts to find out how to create altcoin accounts which might be the main use cases for API usage. Maybe we could even just re-use the createpaymentacct method with different parameters to make altcoin accounts a first class citizen.

@ghubstan Any thoughts on this as well?

@ripcurlx, I agree this should have been added to the api docs long ago.

Do you agree a resolution of this issue deserves its own PR?

ghubstan · 2021-12-22T21:57:53Z

Maybe adding another column with trade state?

I need to think about this for awhile...

I would like to make a "another column with trade state" adjustment in another PR that simply removes the keepfunds and withdrawfunds.

This suggestion would mean a diff from UI: the closing out of a trade cannot be followed up in the API by the UI analog's keepfunds and withdrawfunds cmds. Completion of a trade always automatically results in trade being moved to closed-trades file, and proceeds being moved (or left in) internal bisq wallet. API users can always use the sendbtc and sendbsq cmds to move funds to external addresses.

What do you think of that?

ripcurlx · 2021-12-27T12:44:39Z

Maybe adding another column with trade state?

I need to think about this for awhile...

I would like to make a "another column with trade state" adjustment in another PR that simply removes the keepfunds and withdrawfunds.

This suggestion would mean a diff from UI: the closing out of a trade cannot be followed up in the API by the UI analog's keepfunds and withdrawfunds cmds. Completion of a trade always automatically results in trade being moved to closed-trades file, and proceeds being moved (or left in) internal bisq wallet. API users can always use the sendbtc and sendbsq cmds to move funds to external addresses.

What do you think of that?

Yes, I think that is what @jmacxx implemented here as well: #5870

ripcurlx

ACK - I agree to move the rest into separate PRs.

ghubstan added 18 commits December 1, 2021 16:03

Use calculated offer maker fee, do not re-calculate it

d57d6e9

Use offer instance to determine ownership, not it's id

e3c4089

Calculate offer.makerFee if offer is bsq-swap

33d976c

V1 offer instances have a makeFee value, but not BSQ swap offers. This needs to be investigated. See commit d57d6e9.

Add more readable toString method to MutableOfferPayloadFields

095a495

This makes it easier to compare to EditOfferValidator.toString

Adjust API CorePaymentAccountsService for creating XMR accts

ce8b6df

Allow mkt price lookup for any altcoin from API

742a6f4

Adjust API cli-side CryptoCurrencyUtil & OffersServiceRequest for XMR…

54f815e

… support

Make altcoin offer sorting method names more generic

03b5791

Adjust CLI opts parsers & opt parsing test for XMR support

a45c5fa

Test API XMR support

743ef93

Add CliMain.main(args) tests

65fb8e8

There are no asserts, but helpful for looking at affects of CLI commands from terminal because apitest cases use gRPC service stubs instead of terminal.

Add new regtest apitest/scripts/trade-xmr-simulation.sh

f867f6b

Add a few more checks on editedOffer.getUseMarketBasedPrice()

e2647af

Do not run 2x from gradle terminal cmd

104f11a

Method tests are combined in scenario pkg tests to reduce test harness startup/shutdown time.

Merge branch '5-api-bsqswap-simulation-n-docs-update' into 6-api-xmr-…

0f32fe2

…trading

Merge branch 'master' into 6-api-xmr-trading

5f7fd62

Use string in BigDecimal ctor, and make codacy happy too

98b41cb

ghubstan mentioned this pull request Dec 4, 2021

API BSQ swap simulation script and doc updates #5 #5876

Merged

Merge branch 'master' into 6-api-xmr-trading

705e34e

ghubstan mentioned this pull request Dec 17, 2021

For Cycle 31 bisq-network/compensation#961

Closed

This was referenced Dec 27, 2021

Deprecate API 'getmyoffer' (#7) #5938

Merged

Fix API CLI's '--payment-account-id' opt name, document 'createcryptopaymentacct' (#8) #5939

Merged

ghubstan changed the title ~~API XMR/BTC trading pair support~~ API XMR/BTC trading pair support (#6) Dec 28, 2021

ripcurlx approved these changes Dec 29, 2021

View reviewed changes

ripcurlx added this to the v1.8.1 milestone Dec 29, 2021

ripcurlx merged commit d5dc836 into bisq-network:master Dec 29, 2021

ghubstan deleted the 6-api-xmr-trading branch January 3, 2022 12:34

ghubstan mentioned this pull request Jan 19, 2022

For Cycle 32 bisq-network/compensation#986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API XMR/BTC trading pair support (#6) #5893

API XMR/BTC trading pair support (#6) #5893

ghubstan commented Dec 3, 2021

ripcurlx commented Dec 20, 2021 •

edited

Loading

ghubstan commented Dec 21, 2021

ghubstan commented Dec 21, 2021

ripcurlx commented Dec 22, 2021

ghubstan commented Dec 22, 2021

ghubstan commented Dec 22, 2021 •

edited

Loading

ghubstan commented Dec 22, 2021

ghubstan commented Dec 22, 2021

ghubstan commented Dec 22, 2021 •

edited

Loading

ripcurlx commented Dec 27, 2021

ripcurlx left a comment

API XMR/BTC trading pair support (#6) #5893

API XMR/BTC trading pair support (#6) #5893

Conversation

ghubstan commented Dec 3, 2021

ripcurlx commented Dec 20, 2021 • edited Loading

Trading process

ghubstan commented Dec 21, 2021

ghubstan commented Dec 21, 2021

ripcurlx commented Dec 22, 2021

ghubstan commented Dec 22, 2021

ghubstan commented Dec 22, 2021 • edited Loading

ghubstan commented Dec 22, 2021

ghubstan commented Dec 22, 2021

ghubstan commented Dec 22, 2021 • edited Loading

ripcurlx commented Dec 27, 2021

ripcurlx left a comment

Choose a reason for hiding this comment

ripcurlx commented Dec 20, 2021 •

edited

Loading

ghubstan commented Dec 22, 2021 •

edited

Loading

ghubstan commented Dec 22, 2021 •

edited

Loading