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.

Sign up for GitHub

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

Jump to bottom

Implemented priv_getLogs #686

Merged
merged 7 commits into from
Apr 14, 2020
Merged

Implemented priv_getLogs #686

merged 7 commits into from
Apr 14, 2020

Conversation

lucassaldanha
Copy link
Member

@lucassaldanha lucassaldanha commented Apr 8, 2020 •
edited
Loading

Implemented priv_getLogs. This JSON-RPC method is similar to eth_getLogs, but used to read logs from private transaction receipts.

Closes #628

@lucassaldanha lucassaldanha requested a review from pinges April 8, 2020 22:41
@lucassaldanha lucassaldanha force-pushed the priv_getLogs branch 2 times, most recently from 4af9d1c to 9a59768 Compare April 8, 2020 22:52
@lucassaldanha
Implemented priv_getLogs
c5296a5 
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
@lucassaldanha
Removing comment
b03eca0 
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
pinges
pinges reviewed Apr 14, 2020
View reviewed changes
...main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/parameters/FilterParameter.java
@@ -73,6 +73,14 @@ public Hash getBlockhash() {
return blockhash;
}

public boolean isValid() {
if (!getFromBlock().isLatest() && !getToBlock().isLatest() && getBlockhash() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The filter parameters are a bit confusing. I understand that one of them has to be set, but what takes precedence if a from/to is set and the hash as well? Should that fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

The rules are confusing. Basically, if from/to aren't latest, you can't specify the blockhash.
To be frank, I think we have a bug in this validation. When I compare with the JSON-RPC spec.

The spec says that you can't use fromBlock/toBlock with blockhash at all.

I've talked to Madeline about it and she will raise a JI to review and fix this behaviour.

We are using the exact same logic as eth_getLogs. That's why we decided to keep the same behaviour until we review it. And, if we need to change anything, we change in both methods in the future.

...ava/org/hyperledger/besu/ethereum/api/jsonrpc/internal/privacy/methods/priv/PrivGetLogs.java Show resolved Hide resolved
ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/PrivacyQueries.java Outdated
return Collections.emptyList();
}

final List<PrivateTransactionMetadata> privateTransactionMetadatas =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you just call it "privateTransactionMetadataList"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/query/PrivacyQueries.java Outdated
final List<PrivateTransactionMetadata> privateTransactionMetadatas =
privateWorldStateReader.getPrivateTransactionsMetadata(privacyGroupId, blockHash);

final List<PrivateTransactionReceipt> privateTransactionReceipts =
Copy link
Contributor

Choose a reason for hiding this comment

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

privateTransactionReceiptList?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

...org/hyperledger/besu/ethereum/api/jsonrpc/internal/privacy/methods/priv/PrivGetLogsTest.java Outdated
public void privacyGroupIdIsRequired() {
final JsonRpcRequestContext request = privGetLogRequest(null, mock(FilterParameter.class));

final Throwable thrown = catchThrowable(() -> method.response(request));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should just use assertThatThrownBy() ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Duh! I always forget about it! Done!

...org/hyperledger/besu/ethereum/api/jsonrpc/internal/privacy/methods/priv/PrivGetLogsTest.java Outdated
public void filterParameterIsRequired() {
final JsonRpcRequestContext request = privGetLogRequest(PRIVACY_GROUP_ID, null);

final Throwable thrown = catchThrowable(() -> method.response(request));
Copy link
Contributor

Choose a reason for hiding this comment

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

s.a.

...org/hyperledger/besu/ethereum/api/jsonrpc/internal/privacy/methods/priv/PrivGetLogsTest.java Outdated
privGetLogRequestWithUser(PRIVACY_GROUP_ID, filterParameter, user);
final Throwable thrown = catchThrowable(() -> method.response(request));

assertThat(thrown).isInstanceOf(MultiTenancyValidationException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

assertThatThrownBy?

.../core/src/main/java/org/hyperledger/besu/ethereum/privacy/MultiTenancyPrivacyController.java
@@ -236,7 +236,8 @@ private void verifyPrivateFromMatchesEnclavePublicKey(
}
}

private void verifyPrivacyGroupContainsEnclavePublicKey(
@Override
public void verifyPrivacyGroupContainsEnclavePublicKey(
final String privacyGroupId, final String enclavePublicKey) {
final PrivacyGroup privacyGroup = enclave.retrievePrivacyGroup(privacyGroupId);
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure that the privacyGroup cannot be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, if it is null you won't find any matching privacy group, right?

And in the json-rpc method we are make the privacy group id a required parameter. I don't think we need to be extra defensive here in this internal class.

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/privacy/PrivateWorldStateReader.java Outdated
@@ -46,4 +54,20 @@ public PrivateWorldStateReader(
.flatMap(worldState -> Optional.ofNullable(worldState.get(contractAddress)))
.flatMap(account -> Optional.ofNullable(account.getCode()));
}

public List<PrivateTransactionMetadata> getPrivateTransactionsMetadata(
Copy link
Contributor

Choose a reason for hiding this comment

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

should that be "getPrivateTransactionMetadataList"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

...e/src/test/java/org/hyperledger/besu/ethereum/privacy/MultiTenancyPrivacyControllerTest.java Outdated
@Test
public void verifyPrivacyGroupMatchesEnclaveKeySucceeds() {
final PrivacyGroup privacyGroup =
new PrivacyGroup(PRIVACY_GROUP_ID, Type.PANTHEON, "", "", List.of(ENCLAVE_PUBLIC_KEY1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could create a const privacy group ... The same one is used twice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :)

@lucassaldanha
PR comments
1092f30 
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
@lucassaldanha
priv_getLogs ATs
0e97569 
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
@lucassaldanha
Merge branch 'master' into priv_getLogs
e61cc16 
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
@MadelineMurray MadelineMurray mentioned this pull request Apr 14, 2020
Closed
3 tasks
@lucassaldanha
Fix finals
9f724ed 
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
pinges
pinges reviewed Apr 14, 2020
View reviewed changes
...hyperledger/besu/tests/acceptance/dsl/privacy/contract/CallPrivateSmartContractFunction.java Outdated
final PrivateTransactionManager privateTransactionManager =
new LegacyPrivateTransactionManager(
besu, GAS_PROVIDER, senderCredentials, chainId, privateFrom, privateFor);
PrivateTransactionManager privateTransactionManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can be final

@lucassaldanha
Fix tests
9f1ce56 
Signed-off-by: Lucas Saldanha <lucas.saldanha@consensys.net>
@lucassaldanha lucassaldanha merged commit ba6f656 into hyperledger:master Apr 14, 2020
@lucassaldanha lucassaldanha deleted the priv_getLogs branch April 14, 2020 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinges pinges pinges approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement priv_getLogs
2 participants
@lucassaldanha @pinges