MostroP2P / mostro

Lightning Network peer-to-peer exchange platform on Nostr
https://mostro.network
MIT License
191 stars 32 forks source link

A user can take a sell order multiple times in status waiting-buyer-invoice #311

Closed Catrya closed 2 months ago

Catrya commented 5 months ago

A user can take a sell order multiple times while in status waiting-buyer-invoice ; each time they do so, a new event 38383 will be published.

If they take a range order and choose a fiat amount ¨X¨, when they take it again, even if they choose a different fiat amount, Mostro will ask them to send an invoice for the amount of Sats equivalent to ¨X¨.

I think Mostro updates the fiat amount correctly bc publishes an order discounting that amount of fiat when the trade ends. However, it does not update the amount of Sats each time the buyer takes the order.

Preventing a user from taking the same order twice in the "waiting-buyer-invoice" status helps avoid the other issue.

Example. sell order 100-700 cup 1st time taken: 500 cup (2219 sats) Captura desde 2024-06-25 12-49-21

2nd time taken: 300 cup Captura desde 2024-06-25 12-50-44

3rd time taken: 700 cup Captura desde 2024-06-25 12-51-47

arkanoider commented 3 months ago

@Catrya I don't remember if this issue can be considered closed with #314 , probably you can remind me...

Catrya commented 2 months ago

Sorry, I reopened the issue because I didn't test all cases, but the same user can take the order multiple times.

@arkanoider look this case: If the buyer takes a sales order by range, e.g. 100-1000 CUP and asks for it for 600 CUP, Mostro will ask for an invoice for 3209 SATs.

But if that same buyer takes it again (in status waiting buyer invoice) but for 1000 CUP, Mostro continues asking for an invoice for 3209 SATs.

arkanoider commented 2 months ago

reopened

@grunch The issue is clear we update range-order only when it's settled so we should probably avoid in take_sell.rs to accept take with waiting-buyer-invoice status, as you did here #350 I don't remember now why we have accepted waiting-buyer-invoice in that case maybe @grunch remember if there is a use case for that or we can remove it?

grunch commented 2 months ago

reopened

@grunch The issue is clear we update range-order only when it's settled so we should probably avoid in take_sell.rs to accept take with waiting-buyer-invoice status, as you did here #350 I don't remember now why we have accepted waiting-buyer-invoice in that case maybe @grunch remember if there is a use case for that or we can remove it?

ok let me show you how I use it with mostro-cli currently to add more context and we can find a consensus to make it better:

there is a pending sell order id 272f2442-3af4-4c39-954d-30daf9882eec and I run

mcli takesell -o 272f2442-3af4-4c39-954d-30daf9882eec

then I get a message with the exact amount of the invoice and I run the same command with the new invoice

mcli takesell -o 272f2442-3af4-4c39-954d-30daf9882eec -i lnbcrt26290n1pndwymspp5k80h9d2cqyu9g0rps9v4mr0r62v9f86auyz3avlvcz4tkn0plh2qdqqcqzzsxqyz5vqsp5zf8wqvamxaacn9dvj6ltuwu48zz84wwht5x42xd4qaywzqcy4ucs9qxpqysgq87wxkmxth274p6d2ggpmrlh7rek33ejffmkr7f9y76m8u83lvu3n7vkgptya2rmnvcdfge83ex56ezygfzhhlput750e8ndlu2qkqdsplznvwt

yes, I'm lazy, and I wanted takesell make my life better 😄 , but at the same time the user can use addinvoice, do you think this behavior is not right?

arkanoider commented 2 months ago

reopened

@grunch The issue is clear we update range-order only when it's settled so we should probably avoid in take_sell.rs to accept take with waiting-buyer-invoice status, as you did here #350 I don't remember now why we have accepted waiting-buyer-invoice in that case maybe @grunch remember if there is a use case for that or we can remove it?

ok let me show you how I use it with mostro-cli currently to add more context and we can find a consensus to make it better:

there is a pending sell order id 272f2442-3af4-4c39-954d-30daf9882eec and I run

mcli takesell -o 272f2442-3af4-4c39-954d-30daf9882eec

then I get a message with the exact amount of the invoice and I run the same command with the new invoice.

mcli takesell -o 272f2442-3af4-4c39-954d-30daf9882eec -i lnbcrt26290n1pndwymspp5k80h9d2cqyu9g0rps9v4mr0r62v9f86auyz3avlvcz4tkn0plh2qdqqcqzzsxqyz5vqsp5zf8wqvamxaacn9dvj6ltuwu48zz84wwht5x42xd4qaywzqcy4ucs9qxpqysgq87wxkmxth274p6d2ggpmrlh7rek33ejffmkr7f9y76m8u83lvu3n7vkgptya2rmnvcdfge83ex56ezygfzhhlput750e8ndlu2qkqdsplznvwt

yes, I'm lazy, and I wanted takesell make my life better 😄 , but at the same time the user can use addinvoice, do you think this behavior is not right?

Here it is! Right...we can use addinvoice OR twice takesell commands. The best thing imo should be that a range-order is able to manage more take keeping track of the available amount. The most "easy" one could be blocking users to take a range order while it's not completed...

Catrya commented 2 months ago

imo takesell should only be able to be used to take a sale order, and to add an invoice use addinvoice, but if the bug can be fixed in another way it would be better for everyone

grunch commented 2 months ago

e used to take a sale order,

this sounds reasonable, what do you think @arkanoider ?

arkanoider commented 2 months ago

e used to take a sale order,

this sounds reasonable, what do you think @arkanoider ?

I agree! Sounds more precise to have a command with a specific use, avoid confusion in the user. I am ok doing like that, probably @Catrya PR is ok, I have to look back but could be a merge with no issue. I will review tomorrow morning. Probably double use of takesell was made at the beginning by me and then you added addinvoice.

Let's go with @Catrya implementation!

grunch commented 2 months ago

e used to take a sale order,

this sounds reasonable, what do you think @arkanoider ?

I agree! Sounds more precise to have a command with a specific use, avoid confusion in the user. I am ok doing like that, probably @Catrya PR is ok, I have to look back but could be a merge with no issue. I will review tomorrow morning. Probably double use of takesell was made at the beginning by me and then you added addinvoice.

Let's go with @Catrya implementation!

let's go with this, thanks to @Catrya for putting brain on this, let's test it before merge it