Implement cap 0006 #2016
Implement cap 0006 #2016
Conversation
…m BaseManageOfferOpFrame
…exchangeV10WithoutPriceThresholds
| @@ -25,6 +25,22 @@ namespace stellar | |||
|
|
|||
| using namespace std; | |||
|
|
|||
| static int64_t | |||
There was a problem hiding this comment.
I wonder if those two methods should be merged into one, as getOfferBuyingLiabilities and getOfferSellingLiabilities are mostly called in pairs and it may not be 100% optimal to call exchangeV10WithoutPriceErrorThresholds twice with the same parameters
There was a problem hiding this comment.
Totally agree that it's not 100% optimal because we do the same work twice. But the inefficiency is probably very minor because exchangeV10WithoutPriceErrorThresholds is just a sequence of branches and arithmetic with no database access. I think I'll leave it this way for now since it keeps the code cleaner, but if it ever shows up in profiling as a bottleneck then we can definitely change it because this is purely an implementation detail.
| issuer.pay(a4, cur4, INT64_MAX); | ||
| a4.changeTrust(cur3, INT64_MAX); | ||
|
|
||
| a4.manageOffer(0, cur4, cur3, Price{price.d, price.n}, |
There was a problem hiding this comment.
I see you are testing manageOffer/manageBuyOffer and manageOffer/manageOffer pairs.
Is there a reason to not test manageBuyOffer/manageOffer and manageBuyOffer/manageBuyOffer pairs here?
There was a problem hiding this comment.
There is no need to test the other combinations. There is only one type of OfferEntry, so there is no distinction between the offers created by ManageOffer and ManageBuyOffer once they are in the ledger.
There was a problem hiding this comment.
Looks good in general, a few things missing:
- forgot to rename
ManageOfferOp->ManageSellOfferOp - (missing from CAP006) we should probably rename
CreatePassiveOfferOp->CreatePassiveSellOfferOpfor consistency - need to run clang-format on
Stellar-transaction.x - should
BaseManageOfferOpFramebe renamedManageOfferOpFrameBase? That way, we keep all theManage*Offerfiles together
| } | ||
| if (mManageBuyOffer.offerID == 0 && mManageBuyOffer.buyAmount == 0) | ||
| { | ||
| // Note: This currently does not match the return code of ManageOfferOp. |
There was a problem hiding this comment.
now that I see this, maybe we should make the change, share the code and remove the comment
There was a problem hiding this comment.
Done, and I'll submit a PR to update the CAP accordingly.
| ManageBuyOfferOpFrame::getOfferBuyingLiabilities() | ||
| { | ||
| auto res = exchangeV10WithoutPriceErrorThresholds( | ||
| mManageBuyOffer.price, INT64_MAX, INT64_MAX, INT64_MAX, |
There was a problem hiding this comment.
didn't realize that we're using this price and not the inverse (and that's what the cap says)
There was a problem hiding this comment.
I thought this would be the simplest approach because the price submitted is the price that gets recorded in the ledger. Is there some reason you would prefer the inverse?
There was a problem hiding this comment.
More that it doesn't seem super natural to me as offers are expressed in terms of selling things, so buying implies using the semantics for the other half of the book maybe?
If I have an offer on the book:
- sell 1000 JPY for USD at price 0.009 (manageSellOffer on the "sell JPY for USD" book)
That I try to take entirely (1 is current CAP006, 2 is using the inverse):
- buy 1000 JPY for USD at price 110 (manageBuyOffer on the "sell USD for JPY" book)
- buy 1000 JPY for USD at price 0.009 (manageBuyOffer on the "sell USD for JPY" book)
Right now we only "sell" (the only thing we can do now)
sell 9 USD for JPY at price 110 (manageSellOffer on the "sell USD for JPY" book)
Maybe it's a UI thing. I don't know.
There was a problem hiding this comment.
@jonjove @MonsieurNicolas chipping in here.. this is what I’d expect as a consumer of these APIs:
when dealing with the XLM/USD market:
sell 100 XLM at $0.095
buy 100 XLM at $0.090
converting the same actions above to USD/XLM representation:
sell 9.5 USD at price 11.1111111
buy 9.0 USD at price 10.5263158
i.e. prices (buy and sell) should always be in the quote (counter) currency where for a market X/Y, X is the base currency and Y is the quote currency.
Also, amounts should always be specified in the base currency as in the examples above.
… to CreatePassiveSellOffer
…geOfferOpFrame to ManageSellOfferOpFrame, and CreatePassiveOfferOpFrame to CreatePassiveSellOfferOpFrame
…OfferOpFrame to ManageSellOfferOpFrame, and CreatePassiveOfferOpFrame to CreatePassiveSellOfferOpFrame
| @@ -20,7 +20,7 @@ | |||
| #include "transactions/CreateAccountOpFrame.h" | |||
| #include "transactions/InflationOpFrame.h" | |||
| #include "transactions/ManageDataOpFrame.h" | |||
| #include "transactions/ManageOfferOpFrame.h" | |||
There was a problem hiding this comment.
should also apply adds/renames to the Visual Studio files
There was a problem hiding this comment.
Fixed. Also added ManageBuyOfferOpFrame and ManageOfferOpFrameBase.
|
I spent a few days reviewing this and made a bunch of notes about comments I want to add / renamings for clarity but none of that is pressing, will file later as followups. Thanks to Jon for extensive conversation and clarification, I'm happy with this change (and even think I roughly understand it now!) |
|
r+ 85da215 |
Implement cap 0006 Reviewed-by: MonsieurNicolas
Implementation of https://github.com/stellar/stellar-protocol/blob/master/core/cap-0006.md except that manage offer has not yet been renamed to manage sell offer.