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

Coinbase migration to Advanced Trade #1005

Merged
merged 11 commits into from
Feb 4, 2024
Merged

Coinbase migration to Advanced Trade #1005

merged 11 commits into from
Feb 4, 2024

Conversation

thomasbs17
Copy link
Contributor

Integration of authentication for Coinbase and migration to Advanced Trade

Coinbase Advanced Trade API doc
Coinbase Advanced Trade WS doc

  • Testing: full integration testing using Python 3.11 on Ubuntu 22.04.3 (WSL)
  • Changelog updated
  • All tests passed
  • Flake8 run and all errors/warnings resolved
  • Contributors file updated

Pending questions/points:

  1. I have only implemented the L2_BOOK and TRADES channels.
  2. When subscibing to the TRADES channel, a "snapshot" message is received. Do we want to consume that?
  3. The ticker channel no longer provides best bid/ask. It has been removed for now, I think we could choose from one of the below options for the update:
  • get rid of that callback
  • re-implement channel based on l2 book sub
  • implement the new callback in a different format from the other exchanges

This was referenced Jan 17, 2024
Closed
Closed
bmoscon
bmoscon requested changes Jan 24, 2024
View reviewed changes
CHANGES.md Outdated
@@ -1,5 +1,8 @@
## Changelog

### 2.5.0
* Update: transitioned from Coinbase Pro (retired) to Coinbase Advanced Trade
Copy link
Owner

Choose a reason for hiding this comment

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

the next version is 2.4.2

Copy link
Owner

Choose a reason for hiding this comment

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

sorry, 2.4.1. Can you move this line under the existing 2.4.1 release notes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cryptofeed/exchanges/coinbase.py Show resolved Hide resolved
cryptofeed/exchanges/coinbase.py Outdated Show resolved Hide resolved
@thomasbs17
Copy link
Contributor Author

@bmoscon PR updated to account for your review

@thomasbs17 thomasbs17 requested a review from bmoscon January 26, 2024 02:58
@bmoscon bmoscon merged commit df692b6 into bmoscon:master Feb 4, 2024
1 of 4 checks passed
leftys added a commit to leftys/cryptofeed that referenced this pull request Nov 30, 2024
@leftys
Coinbase migration to Advanced Trade (bmoscon#1005)
b64243a 
* Updated Coinbase: from Coinbase Pro to Advanced Trade

* updated AUTHORS.md

* finalized Coinbase updates

* flake8

* bug and typo fix

* bug fix

* bug fix

* Removed seq_no as per doc: Subscribe to the level2 channel to guarantee that messages are delivered and your order book is in sync.

* updates based on PR comments

* updated CHANGES.md based on PR comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmoscon bmoscon bmoscon 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
@thomasbs17 @bmoscon