WIP PR -- Hyperdrive Matching Engine V2 on top of mint() & burn() #1225
Conversation
Sean329
requested review from
cashd,
dpaiton,
jalextowle,
jrhea,
mcclurejt,
sentilesdal and
slundqui
as code owners
| hyperdrive); | ||
|
|
||
| // Update order bond amount used again to be accurate | ||
| orderBondAmountUsed[order1Hash] += bondAmount - bondMatchAmount; |
There was a problem hiding this comment.
Shouldn't this just be:
| orderBondAmountUsed[order1Hash] += bondAmount - bondMatchAmount; | |
| orderBondAmountUsed[order1Hash] += bondAmount; |
There was a problem hiding this comment.
I see. This is correcting for what was done earlier. Is there a reason not to just do this once? The bondAmount will probably be lower than the bondMatchAmount, so I think that this could fail due to a lack of liquidity.
There was a problem hiding this comment.
Hi Alex, in lines# 90 and 91 I've already updated the mapping like
orderBondAmountUsed[order1Hash] += bondMatchAmount;
orderBondAmountUsed[order2Hash] += bondMatchAmount;
And, I do this here before the execution really goes into the if(xxx){_handle...} blocks mainly to follow the CEI pattern to prevent reentrancy. I know there is a reentrancy guard modifier for this function, but the guard won't for cross-contract reentrancy. So I tend to use both the guard and the CEI patter. Therefore, if we have the Line#90 and 91, then later I will need something like this orderBondAmountUsed[order1Hash] += bondAmount - bondMatchAmount; to revise the update (account for the differences, and ofc the real bondAmount must always be larger than bondMatchAmount as the mint checks the min output. So there shouldn't be underflow risk here)
There was a problem hiding this comment.
I see. This is correcting for what was done earlier. Is there a reason not to just do this once? The
bondAmountwill probably be lower than thebondMatchAmount, so I think that this could fail due to a lack of liquidity.
Can it be? The bondMatchAmount is passed into the mint function call as the min output guard, so can the output of bondAmount still be lower than bondMatchAmount? I thought it couldn't, otherwise the mint just fails?
There was a problem hiding this comment.
I need to think more about this second point. It’s late here, but I’ll take another look in the morning.
Sean329
changed the title
Sean329
force-pushed
the
sean/feature/match-engine
branch
from
08059ba to
a3de4b1
Compare
Hyperdrive Gas Benchmark
This comment was automatically generated by workflow using github-action-benchmark. |
| /// @dev The amount to be used in the trade. In the case of `OpenLong` or | ||
| /// `OpenShort`, this is the amount of funds to deposit; and in the | ||
| /// case of `CloseLong` or `CloseShort`, this is the min amount of | ||
| /// funds to receive. | ||
| uint256 fundAmount; | ||
| /// @dev The minimum output amount expected from the trade. In the case of | ||
| /// `OpenLong` or `OpenShort`, this is the min amount of bonds to | ||
| /// receive; and in the case of `CloseLong` or `CloseShort`, this is | ||
| /// the amount of bonds to close. | ||
| uint256 bondAmount; |
There was a problem hiding this comment.
I think we talked about framing this in terms of price (or rate) rather than in terms of the fund amount and bond amount. It's fine to do this later, but I am curious on your thoughts on this change. What are the pros and cons from the implementation perspective as you see it.
There was a problem hiding this comment.
I thought about it for a while and asking the traders to directly provide fundAmount and bondAmount makes it easier for the matching engine to code. The price/rate is the consideration in the traders' mind, and, in the end for whatever decision they've made, the price/rate can all be reflected in the combination of these 3 params:
- input amount (money or positions)
- min output amount (positions or money)
- maturity time of the output or input positions
So, I put fundAmount , bondAmount , minMaturityTime and maxMaturityTime these four elements in the orderIntent. With them, the matching engine code can be straight forward; and, by carefully selecting the values for these four elements, the trader should be able to convey their price expectations.
Let me know if this makes sense?
There was a problem hiding this comment.
Works for me. Would be good to poke Mihai about it again.
jalextowle
left a comment
jalextowle
left a comment
There was a problem hiding this comment.
LGTM once the comments are addressed and CI is passing
Sean329
force-pushed
the
sean/feature/match-engine
branch
from
2cda1a2 to
932d6b5
Compare
… trader's address
…solidity code styling
…dity code styling
Sean329
force-pushed
the
sean/feature/match-engine
branch
from
932d6b5 to
fdc3d30
Compare
|
Another more comprehensive PR #1239 will be merged into main. This PR and its branch shall be discarded. |
resolves #1222
The
matchOrdersfunction will be called by a 3rd party when he finds two hanging orders can actually be matched. So, both of these 2 orders are actually the maker, and the surplus goes to the 3rd party caller (frontrun competition possible).The
fillOrdersfunction will be called by the taker directly. The take does not need to sign a fullorderIntent. He only grabs the maker'sorderIntentand provide some other particular inputs along to the function call.Checklist before finalization:
asBase==falseas the fund token as well;