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.

Sign up for GitHub

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

Jump to bottom

Extract reused functions into utility packages #3

Merged
merged 6 commits into from
Jan 21, 2022
Merged

Extract reused functions into utility packages #3

merged 6 commits into from
Jan 21, 2022

Conversation

zpv
Copy link
Contributor

@zpv zpv commented Jan 20, 2022

What?

This PR extracts various duplicated utility functions from our various projects and assembles them into utility packages.

Deribit utilities (@atomicfinance/deribit):

  • composeInstrumentName
    • Builds and format option information into Deribit's instrumentName parameter
  • parseInstrumentName
    • Parse Deribit's instrumentName into expiry, option type, and strike price.

Contract utilities (@atomicfinance/contract):

  • calculatePreFeePremium
  • calculatePostFeePremium

Formatting utilities (@atomicfinance/format):

  • Bitcoin:
    • toBTC
    • toSats
    • roundBTC
  • Number
    • round

Why?

To follow the principles of DRY, and reduce duplicated logic and refactoring costs.
This allows our various projects to use the same logic, and avoid bugs encountered by divergent implementation.

@zpv zpv requested a review from matthewjablack January 20, 2022 16:52
matthewjablack
matthewjablack requested changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@matthewjablack matthewjablack left a comment

Choose a reason for hiding this comment

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

just a few minor suggestions in the PR but looks great overall 🔥

packages/deribit/__tests__/deribit-utils/deribit-utils.spec.ts Show resolved Hide resolved
packages/deribit/lib/index.ts Show resolved Hide resolved
packages/format/__tests__/format-utils/bitcoin-format-utils.spec.ts Show resolved Hide resolved
packages/format/lib/bitcoin.ts Show resolved Hide resolved
packages/deribit/lib/index.ts Show resolved Hide resolved
packages/contract/lib/index.ts Outdated Show resolved Hide resolved
@zpv zpv requested a review from matthewjablack January 21, 2022 18:10
matthewjablack
matthewjablack reviewed Jan 21, 2022
View reviewed changes
Copy link
Contributor

@matthewjablack matthewjablack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

just a couple suggestions for test cases

packages/contract/lib/index.ts Outdated Show resolved Hide resolved
@zpv zpv force-pushed the steven/atom-562-extract-reused-functions branch from 96445b3 to ba496b6 Compare January 21, 2022 19:45
@zpv zpv force-pushed the steven/atom-562-extract-reused-functions branch from ba496b6 to 51c35b5 Compare January 21, 2022 19:46
@zpv zpv requested a review from matthewjablack January 21, 2022 19:47
matthewjablack
matthewjablack approved these changes Jan 21, 2022
View reviewed changes
Copy link
Contributor

@matthewjablack matthewjablack left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@zpv zpv merged commit cd32cc8 into master Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewjablack matthewjablack matthewjablack approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zpv @matthewjablack