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

Deactivate open offer if trigger price is reached #5001

Conversation

chimp1984
Copy link
Contributor

Implements #4998

Based on #4993

Copy link
Contributor

@ghubstan ghubstan left a comment

Choose a reason for hiding this comment

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

I do see the offer disabling trigger price in the edit offer view, [EDIT] and can edit it.

But I can't see the disabling trigger price in the published offer views (below). Squeezing it into the list view might not be feasible, but adding it to the other two might be trivial.

Confirm Offer View..
where-is-trigger-price

Published Offer List View...
where-is-trigger-price-in-offer-list

Published Offer Detail View...
where-is-trigger-price-in-published-offer-detail

@@ -0,0 +1,151 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I instinctively need to ask if PriceUtil could be moved to core. I see dependencies on ChatView.log and ui validators, but much of the util logic could be made available to the api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I was thinking the same... was just to lazy to separate pure domain code with formatting stuff which is using desktop scope classes... But yes we should refactor that.

@chimp1984
Copy link
Contributor Author

I was in the process to add a field there but as it only gets the offer passed but not the openOffer I was not sure... But probably its better to add it as well.
The triggerPrice is a field in openOffer not in Offer. Offer will be a part of trade and I think trigger price is a local setting of the user not a part of the mutual data which will be shared with the peer.

There is a trigger price column in openOffersView but it is hidden at default stage width as there is not enough space. If you scale larger it will get added. Similar we use in closed trades, there are a few detail columns only shown when width is larger. I am aware that many users might miss those hidden features, but have not found a good solution yet for it...
Maybe some icon on the table indicating that the table changes on resize, but as that is not a common UI feature I fear the icon will be just confusing...

@chimp1984
Copy link
Contributor Author

Also note that I changed/fixed the above/below term in the price field description. Before it was only changing for BUY/SELL but not for fiat/altcoin. But as fiat price and altcoin price have inverted logic I applied that there as well.
E.g. For a BTC seller a higher fiat price is good but a lower altcoin price.

@ghubstan
Copy link
Contributor

It's good that I can edit the trigger price...
I am (reg) testing, so far so good.
I wonder how difficult it would be to automatically re-enable an offer after the price goes back into the "enable range".
By the way, we shouldn't be doing this today ;-/

@chimp1984
Copy link
Contributor Author

Screen Shot 2020-12-25 at 00 33 23

Screen Shot 2020-12-25 at 00 09 10

Screen Shot 2020-12-25 at 00 34 08

@chimp1984
Copy link
Contributor Author

I wonder how difficult it would be to automatically re-enable an offer after the price goes back into the "enable range".

Yes was also wondering if that might be good... Not sure also if we should show a popup once a offer gets deactivated. In th list view the shild icon gets yellow in that case. If price is still outside of permitted range the offer also cannot be re-activated directly (would trigger deactivation at next price update) but the user need to edit the offer... Not 100% sure if that is clear to users...

Also we do support that feature only for % based price, if you use fixed price you cannot set it as well if there is no price feed like BSQ. Wondering if it would make sense to allow it for those as well?

By the way, we shouldn't be doing this today ;-/

Haha... I don't care much about xmas and holidays...

@chimp1984
Copy link
Contributor Author

If offer got deactivated user need to edit offer and see the validation error:
Screen Shot 2020-12-25 at 12 36 40

@ghubstan
Copy link
Contributor

Haha... I don't care much about xmas and holidays...

We need time off just like religious people ;-)

Not sure also if we should show a popup once a offer gets deactivated.

Not having one is OK for me.

Wondering if it would make sense to allow it for those as well?

Yes, but maybe put that in another PR?

@ghubstan
Copy link
Contributor

ghubstan commented Dec 25, 2020

the shield icon gets yellow in that case..

Ah, I see. Didn't know what that icon meant but the tooltip explains. Good, better that taking up more real-estate with a 'Trigger Price' column header. I think it's fine the way it is.

Copy link
Contributor

@ghubstan ghubstan left a comment

Choose a reason for hiding this comment

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

Tested ACK

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 based on #5001 (review)

@ripcurlx ripcurlx added this to the v1.5.3 milestone Dec 28, 2020
@ripcurlx ripcurlx merged commit 976caeb into bisq-network:master Dec 28, 2020
@chimp1984 chimp1984 deleted the deactivate-open-offer-if-trigger-price-is-reached branch December 28, 2020 17:01
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