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

Clarify meaning of API/Offer related % fields [#6] #6065

Merged

Conversation

ghubstan
Copy link
Contributor

This change fixes some inconsistencies in offer related API fields representing percentages:

  • Rename double marketPriceMargin -> marketPriceMarginPct.
  • Rename double buyerSecurityDeposit -> buyerSecurityDepositPct.

In both cases, these fields' values represent "literal" percent values, e.g., 2.55 means 2.55%.

It also fixes inconsistencies between input-pct values and output-pct values. For example, when createoffer was passed a marketPriceMargin=1.0 (%) param, the getoffer response contained an unexpected decimal representation of the pct literal: marketPriceMargin=0.01. Now createoffer input equals getoffer output: 1.0 (%).

Summary of commits:

  • Disambiguate grpc.proto offer pct literal field names.
  • Adjust GrpcOffersService to new .proto *Pct field names.
  • Adjust core api pkg classes to new .proto *Pct field names.
  • Adjust cli module to new .proto *Pct field names.
  • Adjust apitest cases to new .proto *Pct field names.

Based on branch 5-change-tradeinfo-proto-prc-n-vol-to-string, PR #6062

This commit adds comments to grpc.proto to be pulled into a new API
reference doc.  Some of the comments highlight issues to be resolved
in following PRs.

The main focus is on documenting gRPC Offers service definitions in
grpc.proto:

- Comment each field in the Offers service's messages.
- Add TODO comments about fixing field defs with usability &
  consistency problems.

Other API reference doc related changes to grpc.proto:

- Comment each gRPC service's rpc method definitions, however briefly.
- Improve some existing rpc method comments.
We will consider each problematic number field one at a time.

Note: changing field types in following commits will break
the API's backward compatibility, but devs agree it's OK since
it is still in beta.
The new console output formatting (tbl) api is working, and the
deprecated classes just add maintenance work.

Update affected test cases.
Much less ambiguous field value at cost of breaking backward compat in API.
Quite a bit of refactoring.  A lot of number-string formatting was removed.
Rename double marketPriceMargin -> marketPriceMarginPct.
Rename double buyerSecurityDeposit -> buyerSecurityDepositPct.
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.

utACK - Changes are looking fine 👍

@ripcurlx ripcurlx added this to the v1.8.4 milestone Feb 28, 2022
@ripcurlx ripcurlx merged commit a73ee03 into bisq-network:master Feb 28, 2022
@ghubstan ghubstan deleted the 6-disambiguate-api-pct-field-names branch February 28, 2022 13: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.

2 participants