multi: specify lots as a parameter to RequiredOrderFunds#594
Conversation
| // not be returned in subsequent calls to Fund or calculated in calls to | ||
| // Available, unless they are unlocked with ReturnCoins. | ||
| FundOrder(uint64, *dex.Asset) (Coins, error) | ||
| FundOrder(value, lots uint64, assetInfo *dex.Asset) (Coins, error) |
There was a problem hiding this comment.
Since this can be confusing ("why do I have to specify value and lots?"), the API could frame this as, say, maxSwaps instead of lots since what this really gets to is maximum number of swap contract txns to account for in funding the fees.
I suspect that we neglected the full relevance of the fields of the dex.Asset (coin.LotSize doesn't apply to buy orders) because we didn't spell out just what they were for and which fields were used and how. So as an alternative to passing dex.Asset, we could rethink the FundOrder api to possibly use a OrderFundSpec or ContractAsset struct with just the relevant fields named explicitly for their purpose in funding an order. With this, value/maxSwaps could be a separate arg or part of this hypothetical struct. But if we document FundOrder in detail about the purpose of the passed dexAsset, which fields are pertinent to order funding, and any other args, then I have no problem with continuing to use dex.Asset.
buck54321
force-pushed
the
fix-req-funds
branch
2 times, most recently
from
d9b77eb to
44922d5
Compare
|
Some minor conflicts from the funding mutex changes. |
|
OK to squash if that makes the rebase cleaner. |
chappjc
left a comment
chappjc
left a comment
There was a problem hiding this comment.
All good. Just needs the minor conflict resolution.
use struct for FundOrder arguments tests and improved logging move original coin logging up lower case new error messages in Trade
buck54321
force-pushed
the
fix-req-funds
branch
from
908d823 to
b3d399a
Compare
2 participants
RequiredOrderFundswas calculating the maximum number of possible transactions (maxSwaps= # lots) internally based on the order quantity and the lot size. But for the quote asset, the maximum number of possible transactions is not based on the quote asset lot size, it's based on the rate-equivalent quantity of base asset and it's lot size. This was causing @chappjc's over-allocation from here, because the BTC lot size was set relatively low. This could also go the other way, and under-allocate, potentially causing trouble.Rule of thumb: Lot size is only meaningful for the base asset.
I'm leaving this in draft for a minute while I look it through and think about testing.