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

feat: websocket for sending position/trade updates #2143

Merged
merged 3 commits into from
Mar 10, 2024

Conversation

bonomat
Copy link
Contributor

@bonomat bonomat commented Mar 4, 2024

This new websocket will send trade/position updates as seen from the coordinator's perspective.

No need to do an indepth review, but please let me know what you think about this approach and I'll continue

@bonomat bonomat requested a review from holzeis March 4, 2024 08:19
@bonomat bonomat force-pushed the feat/positions-websocket branch 3 times, most recently from d7f0408 to 7a5361c Compare March 5, 2024 01:49
@bonomat bonomat marked this pull request as ready for review March 5, 2024 03:53
@bonomat bonomat requested a review from luckysori March 5, 2024 03:53
@bonomat bonomat force-pushed the feat/positions-websocket branch 2 times, most recently from 456b717 to 1e2be0e Compare March 5, 2024 04:07
@holzeis
Copy link
Contributor

holzeis commented Mar 5, 2024

It's probably the safest and easiest, but it seems wrong to put the logic around the takers positions.

IMO we should also reflect a position with the matched maker upon every successful trade.

Meaning we would have a consolidated position with the maker for all matched orders.

This would also make the api a bit nicer as a maker could ask for the coordinators point of view of his position and sync up his state.

It would also allow us to scale to multiple makers.

Also the api wouldn't have to be an admin api if we would verify the users signature.

@holzeis
Copy link
Contributor

holzeis commented Mar 5, 2024

I think in terms of maintenance it would probably also be easier to have a position with the maker, which we can patch as needed without affecting a takers position.

@bonomat
Copy link
Contributor Author

bonomat commented Mar 6, 2024

It's probably the safest and easiest, but it seems wrong to put the logic around the takers positions.

IMO we should also reflect a position with the matched maker upon every successful trade.

Meaning we would have a consolidated position with the maker for all matched orders.

This would also make the api a bit nicer as a maker could ask for the coordinators point of view of his position and sync up his state.

I'm not sure I understand what you are saying, because I believe that's what I'm doing?

Imho our matching logic should change though in a way that it either

  1. takes our orders, or
  2. matches against another maker

For the purpose of this websocket, we then should only care about all positions against 1 as all positions in 2 should be automatically balanced.

It would also allow us to scale to multiple makers.

Imho, there should always be just 1 maker for 1.

Also the api wouldn't have to be an admin api if we would verify the users signature.

@bonomat
Copy link
Contributor Author

bonomat commented Mar 8, 2024

@luckysori / @holzeis : please don't forget to review, I'd love to get this into the next release if possible. I don't think it's a controversial change as it shouldn't affect the users at all.

@holzeis
Copy link
Contributor

holzeis commented Mar 8, 2024

I'm not sure I understand what you are saying, because I believe that's what I'm doing?

I am referring to this bit 1e2be0e (#2143).

We are aggregating all open takers positions (on the maker side), instead I think we should have one position with that particular maker and provide updates on that one. e.g. if we'd have multiple makers there would be a position per maker.

It's pretty close to what you have done already, but my proposal would be to not depend the updates on the takers positions, but rather aggregate the position we have with one maker on the coordinator side. And the maker should get updated based on this position, instead of getting all open takers positions. (then this api could also be public as you'd only get updates to your position. btw. that may be also useful with takers to sync up the apps position with the coordinators)

Having the ability to reflect a position with a maker (like we do with a taker) would also make the api a bit nicer IMO. As we do not have to share "InternalPositionUpdates", but can share position updates with a particular user / maker.

pub type ContractId = [u8; 32];

#[derive(Clone, PartialEq, Debug, Serialize, Deserialize)]
pub enum PositionState {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I think this is a bad idea, because we are just exposing the internal state of the coordinator to the maker. Inevitably, we will have to iterate on this model (we already know this work is coming) and this will break the maker. Breaking changes across repositories are particularly annoying to deal with.

I don't think the maker should need all this context to make decisions. I would definitely introduce a dedicated model for the new websocket API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What in particular is a bad idea, moving it into commons ?
Eitherway, it was not necessary after all and I've dropped this commit.

Copy link
Contributor

@luckysori luckysori Mar 11, 2024

Choose a reason for hiding this comment

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

I think sharing an internal position model with the maker is a bad idea. Glad to hear it is not necessary though.

@bonomat
Copy link
Contributor Author

bonomat commented Mar 10, 2024

I'm not sure I understand what you are saying, because I believe that's what I'm doing?

I am referring to this bit 1e2be0e (#2143).

We are aggregating all open takers positions (on the maker side), instead I think we should have one position with that particular maker and provide updates on that one. e.g. if we'd have multiple makers there would be a position per maker.

It's pretty close to what you have done already, but my proposal would be to not depend the updates on the takers positions, but rather aggregate the position we have with one maker on the coordinator side. And the maker should get updated based on this position, instead of getting all open takers positions. (then this api could also be public as you'd only get updates to your position. btw. that may be also useful with takers to sync up the apps position with the coordinators)

Ah, gotya, I'll update the API to only return for the subscribed maker. (although there shouldn't be more than one maker imho).

Having the ability to reflect a position with a maker (like we do with a taker) would also make the api a bit nicer IMO. As we do not have to share "InternalPositionUpdates", but can share position updates with a particular user / maker.

I would like to avoid mixing it with the orderbook-based websocket which gives updates to the trader as these positions are bound to DLCs and the one I'm interested here may not.

@bonomat
Copy link
Contributor Author

bonomat commented Mar 10, 2024

Ah, gotya, I'll update the API to only return for the subscribed maker. (although there shouldn't be more than one maker imho).

Correction, I think this goes into the wrong direction:

In this websocket I'm actually interested in the coordinator's position update, not for a particular maker. That's why I've also put it behind the admin api.

Our current data model as a position currently is defined as having a DLC with a trader. Trader can be a maker or a taker. If we have a maker who also has a DLC with our coordinator, the overall position of the coordinator should be 0.

That's why I need to aggregate all trader's positions as all trades go against the coordinator.

@bonomat
Copy link
Contributor Author

bonomat commented Mar 10, 2024

Ah, gotya, I'll update the API to only return for the subscribed maker. (although there shouldn't be more than one maker imho).

Correction, I think this goes into the wrong direction:

In this websocket I'm actually interested in the coordinator's position update, not for a particular maker. That's why I've also put it behind the admin api.

Our current data model as a position currently is defined as having a DLC with a trader. Trader can be a maker or a taker. If we have a maker who also has a DLC with our coordinator, the overall position of the coordinator should be 0.

That's why I need to aggregate all trader's positions as all trades go against the coordinator.

Long story short, I think my here proposed API/approach is correct :)

Over this websocket we are sending updates about new trades. This means, if a user opens a new position or closes a position, an update is sent out over this websocket. The update includes an incremenatl change but also the total position state as reference. A client listening on this can then reason on this data.
When receiving an order update, we only want to retain `open` orders. Expired, deleted, matched, etc are not of interested in the app anymore.
@holzeis
Copy link
Contributor

holzeis commented Mar 10, 2024

Our current data model as a position currently is defined as having a DLC with a trader. Trader can be a maker or a taker. If we have a maker who also has a DLC with our coordinator, the overall position of the coordinator should be 0.

That's why I need to aggregate all trader's positions as all trades go against the coordinator.

That makes sense to me. Thanks for explaining. 👍

Copy link
Contributor

@holzeis holzeis left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bonomat bonomat added this pull request to the merge queue Mar 10, 2024
Merged via the queue into main with commit df45d39 Mar 10, 2024
20 checks passed
@bonomat bonomat deleted the feat/positions-websocket branch March 10, 2024 07:36
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