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

Update api beta test guide: add 'editoffer' #5577

Merged
merged 47 commits into from
Aug 30, 2021

Conversation

ghubstan
Copy link
Contributor

@ghubstan ghubstan commented Jun 19, 2021

This is the 5th in a series of PRs, starting at #5570
#5576 Should be reviewed/merged before this one.

ghubstan added 30 commits June 12, 2021 18:35
- Define set of editable offer payload fields in MutableOfferPayloadFields.

- Move bulk of EditOfferDataModel#onPublishOffer logic to OfferUtil.
- Add editOffer to GrpcOffersService, CoreApi, CoreOffersService.

- Set editOffer call rate meter to 1 / minute.

- Use new EditOfferValidator to verify editOffer params OK.

- Adust getMyOffer(s) rpc impl and OfferInfo model to use OpenOffer
  for accessing activation state and trigger price.
Reduces size of GrpcClient while allowing for additional bot-friendly
variations of the new grpc editOffer method.
Support for editing BSQ offers is in place, but will be added
in another PR.
Optionally displaying an ENABLED column in CLI side getoffer output
depends on the value of offer.isMyOffer, which is passed via new
boolean arguments to the trade & offer pojo builders.
BSQ offers are fixed-price only.  This change blocks an
attempt to change an altcoin offer to a margin price based
offer, or set a trigger price.
And log CLI's getoffer output to see getoffer formatting -- after adding
new ENABLED and TRIGGER-PRICE columns.
Avoid inconsistent CLI output decimal formats across different
systems' default locales.
@ripcurlx
Copy link
Contributor

I just recognized when creating a new offer in the response the enabled state is displayed wrong:

Some async tasks (fully preparing and publishing a new offer) do not finish in time to be included in the server's response to the CLI. The OpenOffer.State {AVAILABLE, ... DEACTIVATED) is one bit of information the CLI does not get from the server in the createoffer response because the new OpenOffer instance has not yet been added to the OpenOfferManager.

I didn't want to lie about the true Enabled state in the createoffer response, that's why you see NO.
But as you see, calling getmyoffer right after createoffer shows the correct Enabled state YES.

It says NO, but it is visible to the other peer already. When requesting the state of the specific offer it does display the correct value.

The CLI is session-less, it only knows that at the instance of offer creation, no OpenOffer.STATE is ready to be passed to the CLI. By the time you become annoyed at seeing Enabled=NO, the OpenOffer has been added to the OpenOfferManager and is published to other peers. About 1-3 seconds after you successfully call createoffer, the correct Enabled value shows up in your getmyoffer console output.

Not great. Would you prefer that the CLI block on createoffer until the OpenOfferManager has the new OpenOffer object? That did not seem like a good idea before now, and still doesn't... what do you think?

Ok - I do understand the reasoning behind this, but from a developer perspective looking at the response I'd think I have to do an additional enable after creating the offer to get it publish. Maybe we could add another state like PENDING? WDYT?

@ripcurlx
Copy link
Contributor

When trying to use the API programatic I recognized that it is not 100% straight forward working with the API response. E.g. parsing through lines like this 'Sell BSQ (Buy BTC) 0.00003800 0,02000000 526.32 Altcoins 2021-07-13T09:31:56Z IRDQJ-30666af2-460b-4f23-94a5-ec632913b8d0-165 You can't simply split for spaces as the first value contains spaces as well. Maybe we could improve the layout so it is easier to work with (provide JSON response by using a parameter?)

To justify and align column values, I have to add quite a bit of right-padding and left-padding, and that's what's making parsing so difficult.

@dmos62 and I discussed this... he also wanted a more machine friendly CLI console output. I think Json is the answer. But that's another project/PR in itself and I don't think it should be rushed into this PR.

I work around this problem in the apitest trading simulation scripts: Look at line 234 in trade-simulation-utils.sh.

gettradedetail() {
    TRADE_DESC="$1"
    # Get 2nd line of gettrade cmd output, and squeeze multi space delimiters into one space.
    TRADE_DETAIL=$(echo "$TRADE_DESC" | sed -n '2p' | tr -s ' ')
    commandalert $? "Could not get trade detail (line 2 of gettrade output)."
    echo "$TRADE_DETAIL"
}

The bash function gettradedetail takes one parameter, the full, raw gettrade CLI output (the header and values rows).
That raw TRADE_DESC variable gets squeezed into TRADE_DETAIL, where multi-space delimiters are replaced by single space delimiters.

Yes, I was able to work with the current format as well, it is just an additional mental hurdle for devs playing around with the API. Sure, no need to squeeze JSON support into this PR as well, but I think it would make the API way more accessible for other devs trying to implement their use cases.

@ripcurlx ripcurlx modified the milestones: v1.7.2, v1.7.3 Jul 16, 2021
sqrrm
sqrrm previously approved these changes Jul 16, 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

@ripcurlx Do you have any more comments on this PR?

@sqrrm
Copy link
Member

sqrrm commented Jul 16, 2021

I don't think the api calls should block. Better if the output would make it clearer that request is being processed, maybe by having a target state or something like that to indicate that something is happening.

@ghubstan
Copy link
Contributor Author

Ok - I do understand the reasoning behind this, but from a developer perspective looking at the response I'd think I have to do an additional enable after creating the offer to get it publish. Maybe we could add another state like PENDING? WDYT?

PENDING in this particular scenario is good. Thanks for such a simple, self-explanatory solution.

@dmos62
Copy link
Contributor

dmos62 commented Jul 16, 2021

Ok - I do understand the reasoning behind this, but from a developer perspective looking at the response I'd think I have to do an additional enable after creating the offer to get it publish. Maybe we could add another state like PENDING? WDYT?

PENDING in this particular scenario is good. Thanks for such a simple, self-explanatory solution.

Sorry to chime in. What does the Enabled column signify? It sounds like it might functionally mean something different than it does semantically. You wouldn't expect it to go from disabled to enabled by itself. Maybe it's a synonym for published? Could there be a semantic conflict in there?

PS: bravo for the work you've done @ghubstan

@ghubstan
Copy link
Contributor Author

What does the Enabled column signify?

Hey D, thanks for the nice comment.

From the UI's open offers view, you can enable/disable an offer -- add/remove the offer from the offer book.
The api's editoffer command has an --enable=true|false option to do the same thing.

@dmos62
Copy link
Contributor

dmos62 commented Jul 16, 2021

What does the Enabled column signify?

Hey D, thanks for the nice comment.

From the UI's open offers view, you can enable/disable an offer -- add/remove the offer from the offer book.
The api's editoffer command has an --enable=true|false option to do the same thing.

Sounds like there's two state variables (at least implicitly). One is a settable variable signifying the state we request the offer to be in, and the other is a read-only variable for what state it actually is in. Thought it might be worth pointing out.

The PENDING solution sounds good. The column is effectively the read-only variable.

ghubstan added 4 commits July 30, 2021 12:02
Use LinkedHashSet to maintain NetworkEnvelope ordering when Connection#onBundleOfEnvelopes
calls listeners.

Connection#onBundleOfEnvelopes builds a set from an ordered list of NetworkEnvelopes,
then calls listeners during set iteration.  The envelope list ordering is lost if a
HashSet is built, but maintained by switching to a LinkedHashSet.

Losing the envelope ordering becomes a problem if the peer receives a RemoveDataMessage
and an AddDataMessage in the same batch of envelopes, and relays them to listeners
in the wrong (random) order.  For example, an API 'editoffer' call may result in the
edited offer being added, then immediately remove from their UI's offer book.
ghubstan added a commit to ghubstan/bisq that referenced this pull request Aug 16, 2021
@ghubstan
Copy link
Contributor Author

File conflicts resolved in child PR #5666

@ripcurlx ripcurlx modified the milestones: v1.7.3, v1.7.4 Aug 23, 2021
@sqrrm sqrrm merged commit 3693728 into bisq-network:master Aug 30, 2021
@ghubstan ghubstan deleted the 05-update-api-beta-test-guide branch August 30, 2021 14:36
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.

4 participants