-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add unit-tests to verify CLOB/AMM offer and strand selection logic with the transfer fee #4642
Add unit-tests to verify CLOB/AMM offer and strand selection logic with the transfer fee #4642
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// as CLOB's offer and can't generate a better quality offer. | ||
// The transfer fee in this case doesn't change the CLOB quality | ||
// because trIn is ignored on adjustment and trOut on payment is | ||
// also ignored because ownerPaysTransferFee is false in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason that ownerPaysTransferFee
is false is because the OwnerPaysFee feature is unsupported. If that ever changes to Supported::yes
, some of these tests will fail. If that's intentional, great! If not, you should explicitly create the Env
objects with a FeatureBitset
that explicitly removes featureOwnerPaysFee
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is intentional!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add that to the explanation? Something like, "These tests are expected to fail if the OwnerPaysFee feature is ever supported. Updates will need to be made to AMM handling in the payment engine, and these tests will need to be updated."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I left another comment asking to update the test description, but I figure I don't need to re-review after you add that.
Could you add that to the explanation? Something like, "These tests are expected to fail if the OwnerPaysFee feature is ever supported. Updates will need to be made to AMM handling in the payment engine, and these tests will need to be updated."
…ted if OwnerPaysFeature is enabled.
@gregtatcam to confirm whether this is ready to merge. If it's waiting for something, please comment to note that here. |
This PR is ready to merge. |
Suggested commit message:
|
This PR was merged as part of #4626. |
High Level Overview of Change
This PR includes only the unit-tests to verify the payment engine logic to select AMM/CLOB offer and to select the best quality strand when the transfer fee is set.
Context of Change
The transfer fee changes the offer quality and therefore has to be considered when selecting CLOB/AMM offer or the best quality strand. The
transfer in
rate can be optimized out since it is applied to both CLOB and AMM offer. Thetransfer out
rate is set to one in case of the cross currency payment. The quality adjustment for the transfer fee in case of the offer crossing is disabled. Therefore the transfer fee adjustment cancels itself out.Type of Change
Test Plan
The unit-tests consider two cases:
Both cases are considered for the cross-currency payment and the offer crossing.
Future Tasks
If
ownerPaysFee
amendment is enabled that the transfer fee adjustment has to be reconsidered.