Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

feat: implement coincap market service (fixes #281) #286

Merged
merged 6 commits into from
Dec 17, 2021

Conversation

Zerquix18
Copy link
Contributor

Hello. This is the implementation of the CoinCap MarketService.

It follows the interface for MarketService and contains unit tests, all should be successful.

Two things to notice:

  1. In the unit test, I'm directly instancing CoinCapService so that the mocks work. Since Coingecko is first in the list of providers, calling the functions in the parent directory causes a problem with the mocks.
  2. I think coincap doesn't support smart contracts, so the coincapservice will throw an error if the CAIP19 is for a smart contract.

Tried to follow your conventions and make the code a similar to the coingecko service as possible
Let me know what you think :)

@Zerquix18 Zerquix18 requested a review from a team as a code owner December 15, 2021 04:08
@Zerquix18 Zerquix18 changed the title Implemente CoinCap MarketService (fixes #281) Implement CoinCap MarketService (fixes #281) Dec 15, 2021
@0xean 0xean linked an issue Dec 15, 2021 that may be closed by this pull request
@0xean 0xean changed the title Implement CoinCap MarketService (fixes #281) feat: Implement CoinCap MarketService (fixes #281) Dec 15, 2021
packages/market-service/src/coincap/coincap.ts Outdated Show resolved Hide resolved
packages/market-service/src/coincap/coincap.ts Outdated Show resolved Hide resolved
packages/market-service/src/coincap/coincap.ts Outdated Show resolved Hide resolved
packages/market-service/src/coincap/coincap.test.ts Outdated Show resolved Hide resolved
@0xdef1cafe
Copy link
Collaborator

hey @Zerquix18 thank you for the PR!

this is a great start, but i added some additional context of what needs to be done to get this working.

happy with what you did with the unit tests, that's the correct way to go about it.

the main thing is implementing the coincap caip adapter so we know how to talk to coincap correctly. unless @natven can confirm the ids are identical this won't work. in the case that they are, we should at least document it.

also, you'll need to lowercase your PR title, then push up a new commit. we use the angular preset for conventional commits https://github.com/conventional-changelog/conventional-changelog/tree/master/packages/conventional-changelog-angular

i think you can do echo "feat: my commit message" | npx commitlint locally in the root of lib to check without pushing.

@Zerquix18
Copy link
Contributor Author

Hi @0xdef1cafe
Thanks for your feedback! I'll be going through it

The only thing I didn't quite understand was the commit title. Do I have to change the commit history and submit a new PR?

@0xdef1cafe
Copy link
Collaborator

Hi @0xdef1cafe Thanks for your feedback! I'll be going through it

The only thing I didn't quite understand was the commit title. Do I have to change the commit history and submit a new PR?

hey - no need for a new PR. if you just change the title to

feat: implement coincap market service

and push any commit (which you will with your changes) it'll work fine :)

looking forward to seeing the changes

@Zerquix18 Zerquix18 changed the title feat: Implement CoinCap MarketService (fixes #281) feat: implement coincap market service (fixes #281) Dec 16, 2021
@Zerquix18
Copy link
Contributor Author

@0xdef1cafe changed the title and added the adapter for coincap. All tests are passing.
Take a look when you have a chance 😄

Copy link
Collaborator

@0xdef1cafe 0xdef1cafe left a comment

Choose a reason for hiding this comment

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

please see comments

@0xdef1cafe
Copy link
Collaborator

@Zerquix18 - hey, this is looking really good, few small cleanup changes requested and we can get this merged pretty soon

@0xdef1cafe
Copy link
Collaborator

@Zerquix18 also lint is failing CI - run yarn lint:fix from the root to fix things automatically.

@Zerquix18
Copy link
Contributor Author

@0xdef1cafe Removed the scripts and ran the lint:fix command

@0xdef1cafe
Copy link
Collaborator

@Zerquix18 looks great, just gonna get one more approval from someone else in the team and we'll get this merged

@0xdef1cafe 0xdef1cafe merged commit 2159a00 into shapeshift:main Dec 17, 2021
shapeshift-ci-bot pushed a commit that referenced this pull request Dec 17, 2021
# [@shapeshiftoss/caip-v1.7.0](https://github.com/shapeshift/lib/compare/@shapeshiftoss/caip-v1.6.1...@shapeshiftoss/caip-v1.7.0) (2021-12-17)

### Features

* implement coincap market service (fixes [#281](#281)) ([#286](#286)) ([2159a00](2159a00))
@shapeshift-ci-bot
Copy link
Member

🎉 This PR is included in version @shapeshiftoss/caip-v1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

shapeshift-ci-bot pushed a commit that referenced this pull request Dec 17, 2021
# [@shapeshiftoss/market-service-v1.6.0](https://github.com/shapeshift/lib/compare/@shapeshiftoss/market-service-v1.5.2...@shapeshiftoss/market-service-v1.6.0) (2021-12-17)

### Features

* implement coincap market service (fixes [#281](#281)) ([#286](#286)) ([2159a00](2159a00))
@shapeshift-ci-bot
Copy link
Member

🎉 This PR is included in version @shapeshiftoss/market-service-v1.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

sdmg15 pushed a commit to sdmg15/lib that referenced this pull request Jan 6, 2022
…hift#286)

* Add coincap

* add coincap adapter

* remove tokenId check

* readd tokenId check

* remove scripts to generate marketcap data

* apply linter fixes
sdmg15 pushed a commit to sdmg15/lib that referenced this pull request Jan 6, 2022
sdmg15 pushed a commit to sdmg15/lib that referenced this pull request Jan 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CoinCap MarketService Implementation
5 participants