Skip to content
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

fix: extend trading limits functions to multiple assets #58

Merged
merged 4 commits into from
Jun 7, 2024

Conversation

nvtaveras
Copy link
Contributor

@nvtaveras nvtaveras commented Jun 6, 2024

Description

When trading limit related functionality was introduced it was restricted to only asset0 for each exchange, as at the time no exchange had limits configured on both assets. This is no longer the case since multiple exchanges have limits configured on both, so In such cases, the result of mento.getTradingLimits() is incorrect.

This PR does a few different things:

  1. getTradingLimits() now returns the current limits on both assets
  2. getTradingLimitConfig() and getTradingLimitState() now return an array of TradingLimitsConfig and TradingLimitsState with an additional asset field instead of a single config/state, containing the configs/states for all assets in the exchange.
  3. A fix on the getTradingLimits where now the state of limits with a smaller timeframe is constrained by the state of larger ones. For example, if L1 has been maxed out or has a very small capacity left, L0 should reflect that as well. Similarly if LG was completely maxed out and had a remaining of 0, both L1 and L0 should also be set to 0.

Tested

Wrote a few additional tests and tested this manually using scripts/tradingLimits.ts to fetch the config, state and limits of several exchanges, particularly CELO/eXOF as it's currently maxed out on the CELO side:

configs:

[
  {
    asset: '0x73F93dcc49cB8A239e2032663e9475dd5ef29A08',
    timestep0: 300,
    timestep1: 86400,
    limit0: 6560000,
    limit1: 32800000,
    limitGlobal: 196800000,
    flags: 7
  },
  {
    asset: '0x471EcE3750Da237f93B8E339c536989b8978a438',
    timestep0: 300,
    timestep1: 86400,
    limit0: 20000,
    limit1: 100000,
    limitGlobal: 600000,
    flags: 7
  }
]

state:

[
  {
    asset: '0x73F93dcc49cB8A239e2032663e9475dd5ef29A08',
    lastUpdated0: 1717651318,
    lastUpdated1: 1717651318,
    netflow0: 0,
    netflow1: 0,
    netflowGlobal: 6721731
  },
  {
    asset: '0x471EcE3750Da237f93B8E339c536989b8978a438',
    lastUpdated0: 1717651318,
    lastUpdated1: 1717651318,
    netflow0: 0,
    netflow1: 0,
    netflowGlobal: 600000
  }
]

limits (0x471EcE3750Da237f93B8E339c536989b8978a438 is CELO):

 [
  {
    asset: '0x73F93dcc49cB8A239e2032663e9475dd5ef29A08',
    maxIn: 6560000,
    maxOut: 6560000,
    until: 1717651618
  },
  {
    asset: '0x73F93dcc49cB8A239e2032663e9475dd5ef29A08',
    maxIn: 32800000,
    maxOut: 32800000,
    until: 1717737718
  },
  {
    asset: '0x73F93dcc49cB8A239e2032663e9475dd5ef29A08',
    maxIn: 190078269,
    maxOut: 203521731,
    until: 1893456000
  },
  {
    asset: '0x471EcE3750Da237f93B8E339c536989b8978a438',
    maxIn: 0,
    maxOut: 20000,
    until: 1717651618
  },
  {
    asset: '0x471EcE3750Da237f93B8E339c536989b8978a438',
    maxIn: 0,
    maxOut: 100000,
    until: 1717737718
  },
  {
    asset: '0x471EcE3750Da237f93B8E339c536989b8978a438',
    maxIn: 0,
    maxOut: 1200000,
    until: 1893456000
  }
]

Backwards compatibility

Because the changes to the signature of getTradingLimitConfig and getTradingLimitState these changes won't be backwards compatible and will require a major patch, so I have bumped the version of the package to 1.0.0

Copy link
Contributor

@baroooo baroooo left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@philbow61 philbow61 left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@nvtaveras nvtaveras merged commit cfe5573 into main Jun 7, 2024
1 check passed
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.

3 participants