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

Jump to bottom

Add BTC/BSQ pair offer & trading tests #5359

Closed
wants to merge 27 commits into from
Closed

Add BTC/BSQ pair offer & trading tests #5359

wants to merge 27 commits into from

Conversation

ghubstan
Copy link
Member

And fixed maker and taker fees in trade output.

Also tidy'd up some test cases:

  • Fixed some test names
  • Added comments
  • Loosened some trade state/phase asserts
  • Removed direction string literals
  • Made long literals more readable
  • Right justified offer amount and volume columns in CLI output
  • Added some offer and trade logging to see CLI output formatting

This is the 3rd PR in a series, with the goal of supporting the BTC/BSQ trading pair.
PR #5357 must be reviewed/merge before this one.

Here is the improved CLI output for a Buy BSQ (Sell BTC) offer:

Buy/Sell            Price in BTC for 1 BSQ           BTC(min - max)       BSQ(min - max)  Payment Method  Creation Date (UTC)   ID
Buy BSQ (Sell BTC)              0.00003500  0.07500000 - 0.15000000  2,142.86 - 4,285.71  Altcoins        2021-03-25T22:50:30Z  SENGP7Q-d3744afa-847d-4986-816b-e1e993e4f05d-159

Here is gettrade output for BSQ buyer as maker:

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  
SENGP7Q  BSQ buyer as maker              0.00003500     4,285.71   0.00013975             0.08  YES                YES                     0.15000000  YES       YES           YES               NO

Here is gettrade output for BSQ seller as taker:

ID       My Role              Price in BTC for 1 BSQ  Amount(BSQ)  Tx Fee(BTC)   Taker Fee(BSQ)  Deposit Published  Deposit Confirmed  Buyer Cost(BTC)  BSQ Sent  BSQ Received  Payout Published  Withdrawn  
SENGP7Q  BSQ seller as taker              0.00003500     4,285.71   0.00009116             0.23  YES                YES                     0.15000000  YES       YES           YES               NO

@ghubstan
Add api method createcryptopaymentacct
c3f5669 
This change supports creation of BSQ BLOCKCHAIN payment method accounts.

- Added proto message defs to grpc.proto.
- Added grpc server boilerplate to GrpcPaymentAccountsService.
- Added server impl to CoreApi, CorePaymentAccountsService.
- Added createcryptopaymentacct-help.txt.
- Added CLI side support for new api method.
- Added opt parsing unit tests to OptionParsersTest.

This is the 1st PR in a series, with the goal of supporting the BTC/BSQ trading
pair.  Support for other crypto currency payment accounts will be added later.
@ghubstan
Add cli side help for createcryptopaymentacct
27b0900
@ghubstan
Provide more offer & contract detail available to CLI.
9e035e5 
This change adds offer and trade contract detail to the API's Offer
and Trade protos, and improves CLI output formatting.

- Appended missing fields to OfferInfo proto message:
	uint64 sellerSecurityDeposit = 20;
	string offerFeePaymentTxId = 21;
	uint64 txFee = 22;
	uint64 makerFee = 23;

- Added new api proto messages ContractInfo and PaymentAccountPayloadInfo.
  Lighterweight protos are needed because core Trade/Contract classes are
  not visible to CLI.

- Appended ContractInfo field to api proto message TradeInfo.

- Added proto / model converters for ContractInfo and PaymentAccountPayloadInfo,
  and adjusted OfferInfo & TradeInfo.

- Improved CLI output formatting.  Added more trade detail to CLI's gettrade output,
  and prepared to support BTC/BSQ trading pair.  Note a reviewer is advised to
  look at the CLI outout formatting class files instead getting bogged down in the
  many commit changes.
@ghubstan
Add BTC/BSQ pair offer & trading tests
55f157f 
And fixed maker and taker fees in trade output.

Also tidy'd up some test cases:

- Fixed some test names
- Added comments
- Loosened some trade state/phase asserts
- Removed direction string literals
- Made long literals more readable
- Right justified offer amout and volume columns in CLI output
- Added some offer and trade logging to see CLI output formatting
@ghubstan ghubstan added this to In progress in Ship Bisq Daemon and API Mar 25, 2021
@ghubstan ghubstan changed the title Add BTC/BSQ pair offer & trading tests [WIP] Add BTC/BSQ pair offer & trading tests Mar 25, 2021
@ghubstan ghubstan marked this pull request as draft March 25, 2021 23:20
@ghubstan
Copy link
Member Author

Converted this to draft because I need to adjust an apitest trading simulation script for a gettrade output formatting change.

@ghubstan ghubstan changed the title [WIP] Add BTC/BSQ pair offer & trading tests Add BTC/BSQ pair offer & trading tests Mar 26, 2021
@ghubstan ghubstan marked this pull request as ready for review March 26, 2021 14:14
@ghubstan
Fix maker/taker fee format bug, rename methods
aad998c
@ghubstan
Merge branch '02-api-trade-contract-details' into 03-add-btc-bsq-pair…
20d02eb 
…-tests
@ghubstan
Merge branch 'master' into 01-api-add-createcryptopaymentacct
2d66a59
@ghubstan
Add support for creating instant altcoin payment accounts in api
58c885e 
- Added bool tradeInstant field to proto message def.
- Adjusted core createcryptopaymentacct impl to new tradeInstant request param.
- Adjusted cli side createcryptopaymentacct impl to new tradeInstant request param.
- Fixed CliMain's takeoffer help text (was missing the --payment-account opt).
@ghubstan
Remove space char
4fbdc32
@ghubstan
Merge branch '01-api-add-createcryptopaymentacct' into 02-api-trade-c…
6299dc3 
…ontract-details
@ghubstan
Improve takeoffer output and failure reason messaging
6bde12b 
- Added AvailabilityResultWithDescription proto for better takeoffer failure msgs.
- Added VerifyBsqSentToAddress impl to api, but don't expose to CLI yet.
- Show BSQ Buyer Address in gettrade output (changed cli output formatting classes).
- Fixed api.model.PaymentAccountPayloadInfo altcoin instant acct support bug
@ghubstan
Merge branch '02-api-trade-contract-details' into 03-add-btc-bsq-pair…
a1deb1d 
…-tests
@ghubstan
Make test dispute agent registration a test harness opt.
b838e59 
It was a test case opt, now it occurs by default.
@ghubstan
Implement api VerifyBsqSentToAddressReply service
4424deb
@ghubstan
Define grpc call rate meter for CreateCryptoCurrencyPaymentAccount
b2634aa
@ghubstan
Refactor apitest's bot related classes
da99feb 
A new botsupport package was added, with no dependencies on the test
harness.  The intent is eventually move this botsupport pkg code to a
new gradle subproject called ':bot' which would depend only on the
':cli' subproject (to keep the ':cli' module light).

The future ':bot' subproject would not be shipped, but would give java
devs some api java-bot support and at least one example bot.  A bsq-btc
market maker bot is included in this commit.
@ghubstan
Make codacy happy
6fa4f1a
@ghubstan
Copy link
Member Author

ghubstan commented Apr 1, 2021

I've asked @ripcurlx to loosen up a codacy rule. I think it's an error, claiming I'm using a double in a BigDecimal constructor, but I'm using a String.

@ghubstan
Copy link
Member Author

ghubstan commented Apr 2, 2021

This is the explanation for code duplication the reviewer will find in the bisq.apitest.botsupport.util package.

The botsupport pkg should eventually be moved to it's own gradle subproject, and depend only on :cli. Anticipating this, the botsupport package has no dependencies on anything below :cli.

It does not need to be repeated, but is relevant: there is a general problem around API code re-use due to JFX dependencies in :common. Client side subprojects cannot depend on the :common subproject, and you will find the new BotThread, BotUtilities, FileUtil, etc., are modified copies of the orignals in the common module.

The best solution is to carefully split up the common module into UI and non-UI related modules, as mentioned in the 1 April keybase meeting. But this work is not in the scope of this PR and the API v1 rollout. When this does happen, duplicate code in the botsupport pkg will be replaced by dependencies.

This was referenced Apr 2, 2021
Merged
Closed
@ghubstan
Copy link
Member Author

ghubstan commented Apr 4, 2021

This is not ready for mainnet, and needs to be discussed among other core devs. Closing.

@ghubstan ghubstan closed this Apr 4, 2021
Ship Bisq Daemon and API automation moved this from In progress to Done Apr 4, 2021
@ghubstan ghubstan removed this from Done in Ship Bisq Daemon and API Apr 15, 2021
@ghubstan ghubstan deleted the 03-add-btc-bsq-pair-tests branch February 1, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@ghubstan