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

Limit maximum amount of TWAP parts #1670

Merged
merged 10 commits into from
Jun 19, 2024
Merged

Limit maximum amount of TWAP parts #1670

merged 10 commits into from
Jun 19, 2024

Conversation

iamacook
Copy link
Member

Summary

There can be up to uint256 parts in a TWAP order meaning that we would current request information about each from CoW when mapping.

This limits the number of parts we request up to 11 (confirmed by CoW) - which can be configured by an env. var. (swaps.maxNumberOfParts). If there are >11 parts, the final one is used to fetch the status and token information but it means that executedSellAmount and executedBuyAmount are null as we cannot calculate them.

Changes

  • Add swaps.maxNumberOfParts configuration
  • Update TwapOrderInfo['executedSellAmount' | 'executedBuyAmount'] to be string | null
  • Limit number of parts, fetching only the last if more than the maximum
  • Add appropriate test coverage

@iamacook iamacook self-assigned this Jun 18, 2024
@iamacook iamacook requested a review from a team as a code owner June 18, 2024 14:18
@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9566781013

Details

  • 5 of 10 (50.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 49.253%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 3 8 37.5%
Files with Coverage Reduction New Missed Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 1 28.77%
Totals Coverage Status
Change from base Build 9566099283: -0.04%
Covered Lines: 4071
Relevant Lines: 6663

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9568383771

Details

  • 5 of 10 (50.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 49.231%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 3 8 37.5%
Files with Coverage Reduction New Missed Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 1 28.77%
Totals Coverage Status
Change from base Build 9568377389: -0.04%
Covered Lines: 4071
Relevant Lines: 6665

💛 - Coveralls

Base automatically changed from twap-mapping to main June 19, 2024 08:25
@coveralls
Copy link

coveralls commented Jun 19, 2024

Pull Request Test Coverage Report for Build 9578727585

Details

  • 5 of 10 (50.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 49.231%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 3 8 37.5%
Files with Coverage Reduction New Missed Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 1 28.77%
Totals Coverage Status
Change from base Build 9578682874: -0.04%
Covered Lines: 4071
Relevant Lines: 6665

💛 - Coveralls

// Generate parts of the TWAP order
const parts = this.twapOrderHelper.generateTwapOrderParts({
const _parts = this.twapOrderHelper.generateTwapOrderParts({
Copy link
Member

@hectorgomezv hectorgomezv Jun 19, 2024

Choose a reason for hiding this comment

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

Nit: what do you think about renaming _parts to twapOrderParts (or orderParts, allParts, etc.) instead of using a _ prefix? I think it might be more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I renamed the former to twapParts and then parts to partsToFetch for clarity in a2e114f.

@iamacook iamacook requested a review from hectorgomezv June 19, 2024 10:11
@coveralls
Copy link

coveralls commented Jun 19, 2024

Pull Request Test Coverage Report for Build 9580178858

Details

  • 5 of 11 (45.45%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 49.231%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 3 9 33.33%
Files with Coverage Reduction New Missed Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 1 28.77%
Totals Coverage Status
Change from base Build 9578682874: -0.04%
Covered Lines: 4071
Relevant Lines: 6665

💛 - Coveralls

@iamacook iamacook enabled auto-merge (squash) June 19, 2024 12:12
@iamacook iamacook requested a review from hectorgomezv June 19, 2024 12:12
@coveralls
Copy link

coveralls commented Jun 19, 2024

Pull Request Test Coverage Report for Build 9581748889

Details

  • 5 of 11 (45.45%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.001%) to 49.275%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 3 9 33.33%
Files with Coverage Reduction New Missed Lines %
src/routes/transactions/mappers/common/twap-order.mapper.ts 1 28.77%
Totals Coverage Status
Change from base Build 9579639391: 0.001%
Covered Lines: 4073
Relevant Lines: 6665

💛 - Coveralls

@iamacook iamacook merged commit 689396f into main Jun 19, 2024
16 checks passed
@iamacook iamacook deleted the limit-twap-parts branch June 19, 2024 12:37
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