-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(broker): trading limits #65
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.
cool stuff!!!
- enjoyed the usage of the bit mask for the different trading limit variants
- liked the Balance snapshot and the overall refactoring of the broker test
int48 limit0; | ||
int48 limit1; | ||
int48 limitGlobal; | ||
uint8 flags; |
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.
😎
test/common/TradingLimits.t.sol
Outdated
|
||
/* ==================== State#update ==================== */ | ||
|
||
function test_update_withNoLimit_updatesOnlyGlobal() public { |
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.
I was wondering about the name of the test. When we provide an empty config the config.flag == 0
therefore all the if clauses in update should be false and the netflowGlobal
isn't updated which is also checked in the third assert.
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.
Good catch! At some point I was toying around with the idea of always updating netflowGlobal
in order to actually be "global", so also when the limit isn't active, but in the end, I decided to keep it simple and only update netflow*
when the limit is active.
test/common/TradingLimits.t.sol
Outdated
|
||
function setUp() public { | ||
TradingLimits.State memory _state; | ||
state = _state; |
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.
Is this setup needed? Surely you get the same result from just declaring it?
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.
Yeah, I can't reconstruct how I ended up in this situation 😂. Removed it.
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.
Nice work on this. LGTM
Description
This PR implements trading limits for the broker via the
TradingLimits
library.This library provides two structs:
TradingLimits.State
which holds the counters associated with a trading limit setupTradingLimits.Config
which holds the configuration for a trading limit setup.A trading limit is applied to an asset in a pair. For example, say we have the
cUSD/CELO
pair which has anexchangeId
, if we want to apply a trading limit for cUSD in that pair we configure the trading limits onlimitId = exchangeId ^ cUSDTokenAddress
. Inside of a TradingLimits.Config we can enable 3 trading limits:limit0
tokens everytimespan0
seconds.Other changes
Tested
Related issues
Backwards compatibility
N/A
Documentation
N/A