Skip to content

Conversation

@nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Dec 25, 2023

The internal _fillRelayUSS function handles most of the fill logic and this PR adds useful test infrastructure for future testing.

This PR also includes unit testing logic specific to the external fillUSSRelay function

This function handles most of the fill logic and this PR adds useful test infrastructure for future testing
Signed-off-by: nicholaspai <npai.nyc@gmail.com>
@nicholaspai nicholaspai marked this pull request as ready for review December 26, 2023 20:09
@nicholaspai nicholaspai changed the title improve: Unit tests for _fillRelayUSS() internal logic improve: Unit tests for _fillRelayUSS() internal logic and fillRelayUSS() external function Dec 27, 2023
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Two minor comments

]
);
});
it("fast fill emits correct FillType", async function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I understand, does this fast fill replace the slow fill? Or is this a fresh fill?

If it's fresh, it would be good to add a test to be sure that we can overwrite the slow fill enum with a filled enum.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fast fill can replace a slow fill request or be a fresh fill. there is a specific test for this on line 400: "fast fill replacing speed up request emits correct FillType", PTAL!

@nicholaspai nicholaspai requested a review from mrice32 January 3, 2024 16:24
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@pxrl pxrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I have some comments but they also apply to the subsequent PR (#392), so I've left them over there.

@nicholaspai nicholaspai merged commit bd62c33 into master Jan 4, 2024
@nicholaspai nicholaspai deleted the npai/unit-test branch January 4, 2024 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants