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

Add support for private log subscriptions (Pub-Sub API) #858

Merged
merged 9 commits into from
May 12, 2020

Conversation

lucassaldanha
Copy link
Member

@lucassaldanha lucassaldanha commented May 6, 2020

PR description

• Created the two private subscription-related methods (priv_subscribe/priv_unsubscribe)
• Created a new type of subscription PrivateLogsSubscription
• Updated LogsSubscriptionService to observe new blocks and check for private logs (similar to what is done in the FilterManager).
• Updated WebSocketRequestHandler to deal with InvalidJsonRpcParameters exception
• Some plumbing work to get everything working together!

Fixed Issue(s)

fixes #762

@lucassaldanha lucassaldanha requested a review from pinges May 6, 2020 23:06
@lucassaldanha lucassaldanha changed the title [#762] Add support for private log subscriptions (Pub-Sub API) May 6, 2020
@lucassaldanha lucassaldanha added the privacy private transactions label May 6, 2020
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@pinges pinges left a comment

Choose a reason for hiding this comment

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

I have to look at the tests on Monday ...

final SubscriptionManager subscriptionManager,
final PrivacyParameters privacyParameters) {

Optional<PrivacyQueries> privacyQueries = Optional.empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

could do an if/else or even

privacyQueries = privacyParameters.isEnabled() ? x : y;

Copy link
Member Author

@lucassaldanha lucassaldanha May 10, 2020

Choose a reason for hiding this comment

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

I think the way that it is implemented makes it more readable. PrivacyQueries start empty and, if privacy is enabled, we set it to the proper object.

I think in this case the ternary operator will make it less readable.

public Collection<JsonRpcMethod> methods() {
final SubscriptionRequestMapper subscriptionRequestMapper = new SubscriptionRequestMapper();
final EnclavePublicKeyProvider enclavePublicKeyProvider =
EnclavePublicKeyProvider.build(privacyParameters);
Copy link
Contributor

Choose a reason for hiding this comment

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

We do this in the PrivacyApiGroupJsonRpcMethods as well, but in the PrivJsonRpcMethods we do pass the privacy controller and the enclavePublicKeyProvider into the create() method.
This means that we have several instances of the controller and enclavePKP where we would only need one.
Can we fix that in a nice way?

Copy link
Contributor

@pinges pinges left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
@lucassaldanha lucassaldanha merged commit 06ca344 into hyperledger:master May 12, 2020
@lucassaldanha lucassaldanha deleted the besu-762 branch May 12, 2020 02:50
shemnon pushed a commit to shemnon/besu that referenced this pull request May 23, 2020
)

* Created priv_subscribe and priv_unsubscribe methods

Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
privacy private transactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for private log subscriptions (Pub-Sub API)
3 participants