exchange/order/limits: Migrate to new package and integrate with exchanges

[gloriousCode]

PR Description

This looks like a big PR, but its not really. This is a change being brought over from private work. I thought perhaps it could be helpful for others, but happy to keep it private it people are unhappy with it.

• This PR moves execution limits from something held by an exchange, into a storage service
• Moves all order limits to internal/order/limits, also obviously in a bid to appeal to gk I don't want it to be internally enforced
• It introduces a key package helper to minimise code lines
• It improves test coverage of limits package

Use case

As a well-regarded trader, using multiple instances of an exchange, I wish to share the order execution limits between them.

Potential future(s) plans

Across many exchanges, currency details such as expiry, contract decimals and contract denomination are sent in the same response as limits. It would be useful to store these details along with these limits. I currently store them seperately, but UpdateTradablePairs, UpdateOrderExecutionLimits and GetFuturesContractDetails are all the same endpoint across many exchanges. Even if they aren't they could still push changes down to the one set of local storage.

Type of change

• New feature (non-breaking change which adds functionality)

How has this been tested

• Improved coverage of limits
• Golangci-lint pass

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally and on Github Actions with my changes
• Any dependent changes have been merged and published in downstream modules

[shazbert]

Not bad, just some things I noticed.

[shazbert]

Approved, only some suggestions that won't hold up anything.

[gbjk]

Good work 👍

Minor changes only - Will re-review as soon as you throw them back at me.

[gbjk]

🚧 🌴 You've introduced a Run, but kept the assquire.*f
This applies to everything below.

Suggested change

	require.NoErrorf(t, err, "GetOrderExecutionLimits must not error for %s pair %s", a, p)
	require.NoError(t, err, "GetOrderExecutionLimits must not error")

[gloriousCode]

whoops sorry! I dont think I understood at the time

[gloriousCode]

Rather than going in a manually replacing all those fields since they were all so unique, I redid all TestUpdateOrderExecutionLimits to a standard which doesn't include such assertify f's. Hope it's better, can revert if not. I don't believe testing each field is worthwhile so I just check one.

[gbjk]

🚧 Soz buddy, no, I don't agree.
I'm not against changing the way we do this, but not if it reduces RealWorld coverage.
That is: Lines that we actually assert are doing the right thing, rather than just executed by a test.

So looking again at #1464 which man-handled this into this shape, I think our coverage isn't at the right unit.
We're covering all the Fetch* units implicitly via Update*.
So either:

1. Leave TestUpdate doing a full coverage test
2. Move test coverage for for each type into TestFetch* - This will mean duplicating the common assertions though, and just have TestUpdate testing one thing.

For now, I'd vote that you just revert that change and fix the assert.*f

[gloriousCode]

Unfortunately I don't understand the way you are talking. Are you saying that you want the same amount of assertions on data? There's many changes in that PR and there's a lot of words you are saying despite coverage is the same. Its very hard for me to understand what you want changed to improve things outside of reverting

[gbjk]

Let me be clearer. Sorry if it sounds ruder as a result.

• The fields you removed are not being tested. They need to be.
  ( MinPrice, MaxPrice, etc )
  Please revert the change that removed testing for them.

• "Coverage" isn't the only thing that matters. We need to know that values are correct.
  Example: If someone removes a line that means MaxTotalOrders is not being set, coverage would go up. But it's a break

I'll make a clear suggestion at the line for you, to speed this up.

[thrasher-]

Thanks for making those changes! Just some final changes and this LGTG

[gbjk]

Thanks

Just a couple of 🚧 issues on reduced coverage of pricing limits.
I'm happy to pitch in and move those all to lower unit tests on Fetch* if you'd like a hand.

[gbjk]

🚧 Same as binance, I don't think we should reduce coverage like this.

[gbjk]

Aside: I'm "going with it" that this should move from positive to "not zero" - Assuming that was deliberate and applies to both fields

Suggested change

	assert.NotZero(t, l.PriceStepIncrementSize, "PriceStepIncrementSize should not be zero")
	assert.NotZero(t, l.PriceStepIncrementSize, "PriceStepIncrementSize should not be zero")
	assert.NotZero(t, l.MinimumBaseAmount, "MinimumBaseAmount should not be zero")

[samuael]

ch ACK!

[gbjk]

Oh, I'm very happy. Thank you 🙏

[thrasher-]

Thanks for making those changes!