-
Notifications
You must be signed in to change notification settings - Fork 12
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
Potential display bug in the way mastercoin-tools handles an edge case #53
Comments
Need to create a TMSC case that mimicks the 1C7AmZ7sEHNLTWdoU7eUqXDPRuM6kmt5a7 Double Offers bug to verify display of accepted amount in this case is still broken in m-t. |
Can you please describe what you mean with "double offer bugs"? It's a bit difficult for me to follow what's a bug, what is correct correct or only a display issue, so I looked at this more closely. At first an overview, potential issues based on what I saw and at the end details. Note: I did not look at the reason why there were differences at all and probably wrong assumptions by the actors, but only at the data available now. Probably everything started with the impression of an available balance of 19 MSC which was actually 18 MSC in reality. I think this relates to the three (?) transactions which caused some trouble earlier where the "tx correction due to a bug - blacklist" was used for a short time. Events:
Issue 1: the overview of an address is confusing, because it shows the amounts of the raw transactions, but not the amounts that were traded or available for sale in the first place. To compare:
https://masterchain.info/Address.html?addr=1C7AmZ7sEHNLTWdoU7eUqXDPRuM6kmt5a7¤cy=MSC Issue 2: the raw information about the first expired sale does not match with the raw data I looked at. See part three later for the full details and differences. Issue 3: if my understanding of the UI is correct, then there may be an issue with last sale and the "accepted" amount: It shows:
I would assume that the "requested" amount is what the buyer tries to lock via an accept transaction, so this number matches. The actually "accepted" amount was 0.68023022, but is shown as 1.68023022. The "bought" amount is the amount that was sold in the end. The data is from the following page: Now let's go into the details: Part 1:
According to the validation logs 1C7Am had a balance of 18 MSC before the sell offer, but the raw data of the sell offer itself was for 19 MSC (which is not included in the logs and simply floored to the available balance). Raw:
Masterchain.info shows this offer is for 19 MSC, which is correct in terms of the raw transaction, but doesn't reflect the reality, because only 18 MSC are available to be sold: Masterchest shows a sale of 18 MSC: Part 2: Then the following happened: 13z1JFtDMGTYQvtMq5gs4LmCztK3rmEZga accepted the offer and wants to buy 0.68023020 MSC.
Raw data of this sell accept:
But Masterchain.info shows this accept offer as invalid with the message: "accept offer with missing amount available on sell offer": According to the local logs this seems to be wrong at this point. Let's keep in mind: there is a pending accept of 0.6802302 MSC from 13z1JFtDMGTYQvtMq5gs4LmCztK3rmEZga. A closer look for the raw data used by masterchain.info: https://masterchain.info/tx/f726c8995a7da5e009801958027e3bf7a1123e66134fc713e3b8a8f99e81de85.json
Local log:
Despite the fact that the accept expired nevertheless, let's keep in mind that 0.6802302 are reserved. Masterchest seems to get it right: Part 3: Then there is an accept for 17.3197698 MSC: Raw:
So: two pending sales for 18 MSC in total at the moment. Part 4: There is the payment of 2.59623349 BTC. The price for the MSC was 0.1499 BTC/MSC, thus this should be enough to pay for 17.31976977'9853235490326884589726 MSC (17.31976980 were accepted, so 0.00000002'125... MSC are left unpaid). (Sidenote: paying one more satoshi would equal a theoretical payment for 17,31976984'656... MSC, or an overpayment of the equivalent of 0.0000004 ... MSC) https://blockchain.info/tx/01d9e09b4f92620738d76a2a8428b82c182356ea852f94b8e5b51698e06e7e06 According to Masterchain this was a buy of 17.31976978 MSC (same amount on Masterchest): Note: we should probably be more explicit how floating numbers are treated in general. Matches with the log:
Results at this point: 0.00000002 MSC reserved for 1MKiW1nYprHTmT4LQvtYZgYGutufKAC2Uj (due to the leftover) Part 5: Some time later the accept of 0.68 expires:
Logs are consistent and a bit later the leftover of 2 willetts (!? xD) expires, too:
Sounds a bit cryptic (at least for me), but it appears those 2 bits are added to the amount up for sale. Part 6: Another accept is incoming from the buyer who let the first 0.68.. expire:
As mentioned above, the log shows only the amounts based on available balances etc., and this accept was actually for 1.68023022 MSC: (raw transaction)
This is reflected by the explorers: https://masterchain.info/sellaccept.html?tx=67d6302a45380289de0097afdae8d21a84b0a41221ca14319b3e4cdd8952a53b¤cy=MSC Part 7: In the end this accept was paid, but the buyer assumed it's for 1.68.. MSC and sent 0.25186651 BTC: https://blockchain.info/tx/46886cdcff50dab670d84788d0fe7fccea21f0a90ab91a5bd910b27c001b5272
Also shown correct on the explorers: |
Issue 1 and Issue 3 are display issues, purely; we can't change the Masterchain UI so I think you're stuck up a tree there; I won't advocate changing the underlying data because there isn't a UI besides masterchain that uses it, so semantically it makes no difference. Issue 2: AFAI can tell, you actually can accept or offer any amount in your offer and the UI will display that (amount_accepted or amount_offered), but the protocol will enforce its own rules on your packet based on its knowledge of state (amount_bought) -- This is not good IMO because it can be used to trick users but it is legal within the spec and will likely cause user error in the future tldr I am worried about how state is documented for offers and accepts but haven't found a solution |
I'm of the opinion if we reproduce this in Test, we should be able to have a good test case for fixing the issue, I am looking into this over the wknd |
Michael found this today: https://masterchest.info/lookuptx.aspx?txid=f0eae2719e03907abed76ee8397b99c4073b8994dd3213f9df4e4df325636bab A user who overpaid for some MSC, because of the same bug, see: this is a critical issue, moving prority -- dexx we should fix this A S A P |
I have the impression that I'm missing the point. Can you please summarize the problem that you see? What I totally dislike is the UI, because it's confusing and misleading, but this is not what you are referring to, right? But just another example: https://masterchain.info/Address.html?addr=1QG3Jw5Mf76sQTcvQoe1cqBbX4g5z6ixXb¤cy=TMSC 1QG3 has a balance of 0.01 TMSC, puts up 9000 TMSC for sale, Appearingly and on the first glance 4500 TMSC are already sold and even better: the accept of a buyer of 10000 TMSC is also there: |
The problem I see is specific to how the UI chooses to display certain elements;
example: Seller 1Gv1Gd86ERbJG8PnXiRx4PTiVtnJ9UY9HL has 50.33506052 MSC Seller decides to make a sale offer for the whole amount ( 90e84b2d957fe...) Numerous buyers buy pieces of his offer, one of whom is requesting an amount more than the offer allows (f63b77e0e8ee7f...). Specifically, the buyer is requesting 1MSC, but the actual amount remaining is 0.00000004 on the seller's account. The total valid accepts add up to 51.33506052, and the seller balance is 50.33506052. Why do the valid accepts add up this way? A very malicious-looking bug in the accept code allows users to see their accept as valid for the full inital amount, and misleads them into spending more BTC than they would have with the less misleading information. To see why this is a a problem, consider a shady salesman. A shady salesman bids you on a car. You take his bid at face value, considering the offer a bargain. After signing the contract and handing over your money, he points out a clause that states you paid for only the steering wheel of the car, and not the car itself. You can see the problem now, I'm sure. This is what is happening here, specifically, the user thinks he is getting X MSC for Y BTC, but it really is 1/X. The user here is paying a hefty premium for just the steering wheel, approximately 0.08/0.00000004=2000000% more. |
Thanks, then we are talking about the same thing. This is an UI issue, not a protocol one. Let's pin it down and then see what needs to be fixed, adjusted and improved. From the seller's side:
Say 1QGE has a balance of 0.01 TMSC and puts up a sale of 9000 TMSC for 90 BTC. Low level: only 0.01 TMSC available, so put up 0.01 TMSC for sale for a total price of 9000/90*0,01 = 0,0001 BTC. But ...
From the buyer's side:
16X6 sends an accept order for 4500 TMSC. Low level: only 0.001 TMSC available for sell, so the maximum of 0.001 TMSC is reserved until payment or accept expires.
The spec on payments:
The payment from the example is fine. Masterchest shows what really happens, at least partial: 9000 TMSC sell offer where 0.01 TMSC were available:
So now the real question: Some of those numbers can be adjuested in a way that the user interface may be easier to understand, but this differs greatly from what's done now. I'd go the route zathras goes and rather display the actual numbers instead of the requested ones. What do you think? |
Great work dexx, I think the points that are labelled FAIL are the fixes we need to implement. As far as what we should display, I think keeping the changes minimal will help keep things working smoothly and introduce a minimum of bugs: KEYFIX: Amount accepted: 4500.0 --- CRITICAL FAIL: this is the amount requested, not the accepted amount of 0.01 This has to be fixed at a minimum for this bug to be considered closed The other FAILS are related to mainly whether we choose a MasterChest or MasterChain method of displaying data to the user. Personally, I like giving the user all the information as MasterChain has done, but highlighting the most important parts or offering a tooltip to explain what each field means. I understand however some might find this "too much information" and just prefer the necessary info to make a decision. Technical Addendum: FYI, there's only two fields that needs to be fixed to clear all the FAILS, including the CRITICAL one, and its the k'amount accepted' in the UI and k'amount' field for Sells. On k'amount_accepted': It looks like the wrong field is used on the UI, grazcoin uses 'formatted_amount' instead of 'formatted_amount_accepted', which holds the correct value: https://github.com/grazcoin/mastercoin-tools/blob/master/www/sellaccept.html#L115-L116. Sell Offer changes:
Reasoning: |
Simplified these two changes: For sells, just going to rename the text field, that way its more accurate: https://github.com/grazcoin/mastercoin-tools/blob/master/www/selloffer.html#L107-L108 For accepts, like discussed above, we just need to have the correct field. PR here, let me know what you think, and I'll send it up: https://github.com/faizkhan00/mastercoin-tools/commit/152a79c4a8adf4b835ddb96affbcd60aede879dc |
*Sold TMSC coins: 4500.0 --- FAIL: this amount is what the buyer requested, not what was really sold There's no way for me to fix this that I see without doing a deeper change, since its up to the protocol to determine this value, and I don't want to change the existing data structure to make it in the fix, if possible. Can you see a way? *Bitcoin amount desired: 90.0 -- FAIL: same applies Should this be changed to 'amount_available*price_per_coin' instead? |
Okay DexX, please check out the last 4 commits on graz_master branch off my fork: https://github.com/faizkhan00/mastercoin-tools/commits/graz_master they contain fixes for the following: History: Sold TMSC coins: 4500.0 --- FAIL: this amount is what the buyer requested, not what was really sold Please let me know if the terminology is incorrect to you, and any other concern you might have, thanks. |
Renamed the field for 'bitcoin amount desired' because its too misleading to 'bitcoin amount required' added 1 extra commit |
One moment. |
I think we almost have it! :) Posted my notes here: |
from email thread:
The numbers for 13fb62038d's sell amount are still incorrect/not correctly reflected, which causes an incorrect accept amount to be shown on 67d6302a45380289de0097afdae8d21a84b0a41221ca14319b3e4cdd8952a53b.
The text was updated successfully, but these errors were encountered: