protos(view): add a subaccount filter to the OwnedPositionIds rpc
#4985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erwanor
changed the title
subaccount to OwnedPositionIds rpcsubaccount filter to the OwnedPositionIds rpc
6 tasks
conorsch
requested changes
Jan 16, 2025
There was a problem hiding this comment.
These changes look solid.
One question I have is whether the buf publish action will be able to pickup the change to the protos.
Yes, these changes will get published to BSR automatically, however this PR should target the main branch so that upon merge the workflow will run. Requesting changes:
- Rebase PR against main
- Regen protos after rebase
- Re-target PR to main
Then we should be good to go. I'll handle the point release mechanics in #4988.
2 tasks
conorsch
added a commit
that referenced
this pull request
Jan 17, 2025
…4992) ## Describe your changes This PR resubmits the unmerged changes from #4985, targeting `main` branch. Below is the original submission text for 4985, which is still accurate here. This PR: - adds an `AddressIndex` field to the `OwnedPositionIds` rpc request and response - regenerate the codegen protobufs It doesn't seem important to do the extra work to support filtering in the rust view server. The natural consumers of that information are web interfaces. Market makers using the rust implementation would probably simply integrate it in their own OMS rather than relying on the view service RPC. One question I have is whether the buf publish action will be able to pickup the change to the protos. ## Checklist before requesting a review - [x] I have added guiding text to explain how a reviewer should test these changes. - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > view service changes --------- Signed-off-by: Erwan Or <erwan.ounn.84@gmail.com> Co-authored-by: Erwan Or <erwanor@penumbralabs.xyz> Co-authored-by: Erwan Or <erwan.ounn.84@gmail.com> Co-authored-by: Lúcás Meier <lucas@cronokirby.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
This PR:
AddressIndexfield to theOwnedPositionIdsrpc request and responseIt doesn't seem important to do the extra work to support filtering in the rust view server. The natural consumers of that information are web interfaces. Market makers using the rust implementation would probably simply integrate it in their own OMS rather than relying on the view service RPC.
One question I have is whether the buf publish action will be able to pickup the change to the protos.
Checklist before requesting a review
I have added guiding text to explain how a reviewer should test these changes.
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: