Independent logic for Partial and Full OrderValidation

[bh2smith]

Similar to the Pre-Order Validation introduced in #1169, we offload some of the order validation statements contained in Orderbook.add_order to a specialized struct PostOrderValidator. This makes it possible to test and maintain Order Validation without making changes to the orderbook logic itself.

Test Plan

Added new unit tests for all possible routes in PostOrderValidator.validate() along with tests for helper method min_balance

[codecov-commenter]

Codecov Report

Merging #1176 (7b6cfac) into main (5903b36) will increase coverage by 0.46%.
The diff coverage is 82.53%.

@@            Coverage Diff             @@
##             main    #1176      +/-   ##
==========================================
+ Coverage   66.69%   67.15%   +0.46%     
==========================================
  Files         135      135              
  Lines       27039    27223     +184     
==========================================
+ Hits        18034    18282     +248     
+ Misses       9005     8941      -64     

[nlordell]

I find it weird that post_validate calls pre_validate. From the names I would expect you to have to call both:

validator.pre_validate()?;
// do stuff...
validator.post_validate()?;

Maybe pre-/post- prefixes can be changed (partial-/full-? Maybe something else)?

[bh2smith]

Yes, initially I had a call to each independently, but the only place this is done would be in Orderbook.add_order. I was worried that future refactors might forget that then both need to be called, so I enforced this by calling the pre inside the post. I completely agree that the name could be changed to partial and full or maybe even partial_validate and just validate.

[bh2smith]

Fun fact, I had already mentioned this here #1176 (comment)

[nlordell]

maybe even partial_validate and just validate.

Works for me. @vkgnosis, from your comments, I think you also didn't find the names super clear, do you have any suggestions as well?

[nlordell]

While I think the trait here is cool, I don't see any generic code with something like T: WarpReplayConverting and feels a bit of overkill.

I'm OK with leaving it in though, but maybe makes sense to move it to the api.rs module because its not order validation specific?

[bh2smith]

I think this will become more useful once #1176 comes into play (so I can implement this trait for a struct that is not declared inside this crate). Would that be a good reason for it?

[nlordell]

Personally, I would still not use a trait in that situation (for mostly stylistic reasons), but there isn't anything wrong with doing so feel free to leave it in if you like it. I don't see a strong reason for using or not using a trait either way.

I think a more concrete reason to introduce such a trait would be the ability to get rid of all those *_response (for example create_order_response) methods and introduce a generic one:

pub fn convert_response<T, E>(result: Result<T, E>) -> impl Reply
where
    T: serde::Serialize,
    E: WarpReplyConverting,
{
    // ...
}

[nlordell]

(Not suggesting this be done in this PR).

[bh2smith]

Cool I will defs do this (after).

[bh2smith]

For now I will move the trait to api.rs (i.e. before merging this).

[nlordell]

Looks mostly good. Just some comments.

Its too bad GitHub UI doesn't catch that its a file rename, making it a bit harder to review.

[bh2smith]

Its too bad GitHub UI doesn't catch that its a file rename, making it a bit harder to review.

I could rename it back for now and just do the rename later if you prefer.

[nlordell]

I could rename it back for now and just do the rename later if you prefer.

Its fine. It was mostly me complaining at the Github UI, which does it sometimes even when Git identifies changes as a file rename.

[bh2smith]

I could rename it back for now and just do the rename later if you prefer.

Its fine. It was mostly me complaining at the Github UI, which does it sometimes even when Git identifies changes as a file rename.

Too late... already renamed back... Will change again later.

[nlordell]

It actually made it much easier to review! Tahnks.