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

Remove wallet id filter from view service #3557

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

grod220
Copy link
Contributor

@grod220 grod220 commented Jan 2, 2024

Closes penumbra-zone/web#334

Only one wallet is active at a time for the view service. Therefore, we can remove all of these wallet_id filters for it.

@grod220 grod220 force-pushed the remove-wallet-from-view branch from 9cc3916 to f88c799 Compare January 3, 2024 01:18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Critical thing to review. Proto messages have been updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think RustRover reformats the imports in this way to make publicly exposed exports at the top

Copy link
Member

Choose a reason for hiding this comment

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

kind of noisy but seems fine. there's a way to get cargo fmt to do this but it requires using nightly rustfmt and a custom config and that seemed like an unacceptable DX tax to put on every contributor, but the effect is that our imports don't stay tidy. so occasionally having them all cleaned up seems fine.

Comment on lines -24 to -26
//
// View protocol requests optionally include the wallet id, used to
// identify which set of data to query.
Copy link
Member

Choose a reason for hiding this comment

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

If we did want to have a multi-tenant view service in the future, we could do so by putting the wallet ID in the request headers. This would be reasonably easy to implement on the Rust side. But not important for now, just noting that the option is open.

Copy link
Member

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

LGTM!

@hdevalence hdevalence merged commit ac766cf into main Jan 3, 2024
5 checks passed
@hdevalence hdevalence deleted the remove-wallet-from-view branch January 3, 2024 01:45
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.

Implement filters for StatusStreamRequest
2 participants