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

Refactor data request interfaces into messages #2260

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

faysou
Copy link
Collaborator

@faysou faysou commented Jan 27, 2025

Pull Request

Refactor data interfaces into messages

Type of change

  • New feature (non-breaking change which adds functionality)

How has this change been tested?

Updated existing tests

@faysou faysou closed this Jan 28, 2025
@faysou faysou reopened this Jan 28, 2025
@faysou faysou changed the title Refactor data interfaces into messages Refactor data request interfaces into messages Jan 28, 2025
Copy link
Member

@cjdsellers cjdsellers left a comment

Choose a reason for hiding this comment

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

Thanks for your work here @faysou

I think this is a great improvement:

  • Better symmetry with command messages on the execution side
  • Stronger contract, as request commands must be valid when initialized in Actor
  • Reduces the surface area of the DataClient interface, making them easier to work with

It's also a good sign that even with all the additional message types, there is still more code being removed here than added.

I think there is some change risk for the adapter clients, as test coverage isn't as strong there as the core of the system. I've reviewed them, but it would be great if we could get additional reviews when possible 🙏 :

nautilus_trader/common/actor.pxd Outdated Show resolved Hide resolved
nautilus_trader/common/actor.pyx Show resolved Hide resolved
nautilus_trader/data/messages.pyx Show resolved Hide resolved
nautilus_trader/live/data_client.py Outdated Show resolved Hide resolved
@davidsblom
Copy link
Member

LGTM (dYdX changes)

@sunlei
Copy link
Collaborator

sunlei commented Jan 29, 2025

LGTM (Bybit)

@cjdsellers
Copy link
Member

Thank you @davidsblom and @sunlei

After we resolve some of the comments above, I'm planning to merge this after the next release (which should be this week).

@faysou
Copy link
Collaborator Author

faysou commented Jan 29, 2025

I'm done with the fixes. I had intended to work on doing the same for the subscribe side, but not sure it's really needed, there are less parameters and variability compared to data requests (also I don't want to embark on refactoring thousands of lines again straight away)

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.

4 participants