Added eea_getTransactionCount Json Rpc #1861
Conversation
Exposers a Json Rpc which allows "legacy" users to query for the nonce of a given address, in a group of private users.
|
|
||
| public static class EeaGetTransactionCountJsonBody { | ||
|
|
||
| final String privateFrom; |
|
|
||
| public class EeaGetTransactionCount implements JsonRpcMethod { | ||
|
|
||
| public static class EeaGetTransactionCountJsonBody { |
There was a problem hiding this comment.
Pattern used in the JsonRpc package has the Json POJO split out into their classes and suffixed with Parameters e.g. tech.pegasys.pantheon.ethereum.jsonrpc.internal.parameters, something worth considering?
| import org.junit.Test; | ||
|
|
||
| public class EeaGetTransactionCountTest { | ||
| final Address address = Address.fromHexString("55"); |
| new EeaGetTransactionCount(new JsonRpcParameter(), enclave, privateTransactionHandler); | ||
|
|
||
| final JsonRpcResponse response = method.response(request); | ||
| assertThat(response).isInstanceOf(JsonRpcErrorResponse.class); |
There was a problem hiding this comment.
Are you expecting a specific error message here?
| new EeaGetTransactionCount(new JsonRpcParameter(), enclave, privateTransactionHandler); | ||
|
|
||
| final JsonRpcResponse response = method.response(request); | ||
| assertThat(response).isInstanceOf(JsonRpcErrorResponse.class); |
There was a problem hiding this comment.
ditto, is any JsonRpcErrorResponse acceptable? or do you expecting a specific message?
| import org.apache.logging.log4j.Logger; | ||
| import org.bouncycastle.util.Arrays; | ||
|
|
||
| public class EeaGetTransactionCount implements JsonRpcMethod { |
There was a problem hiding this comment.
Think this class should be name PrivGetLegacyTransactionCount
There was a problem hiding this comment.
Talked to Madeline last night - given this capability is really required of Legacy Privacy - it belongs in the Eea namespace, and thus, the spec will need updating.
There was a problem hiding this comment.
More to the point - priv_ namespace is geared toward the new Privacy Group - so having "priv" and "legacy" in a name is not a valid combination.
| private long determineNonce(final EeaGetTransactionCountJsonBody getTransactionCount) | ||
| throws Exception { | ||
|
|
||
| final String[] groupMembers = |
There was a problem hiding this comment.
Perhaps there should be a method on the PrivateTransactionHandler to get the legacy privacy group?
There was a problem hiding this comment.
I couldn't decide if this is something a PrivateTransactionHandler should know about ...
rain-on
changed the title
| import org.apache.logging.log4j.Logger; | ||
| import org.bouncycastle.util.Arrays; | ||
|
|
||
| public class LegacyNonceProvider { |
There was a problem hiding this comment.
Why does this have the Legacy prefix, rather than Eea?
There was a problem hiding this comment.
Because "Legacy" is the default terminology for things which work with EEA privacy ... (i.e. internally the Privacy is deemed LEGACY or PANTHEON.
| this.privateTransactionHandler = privateTransactionHandler; | ||
| } | ||
|
|
||
| public long determineNonce( |
There was a problem hiding this comment.
Is there a more narrow exception to throw instead of Exception?
There was a problem hiding this comment.
alas - this is an effect of Encalve throwing Exception ... this behaviour is mimic'd from other Priv Json RPC ...
| final long nonce = nonceProvider.determineNonce(privateFrom, privateFor, address); | ||
| return new JsonRpcSuccessResponse(request.getId(), Quantity.create(nonce)); | ||
| } catch (final Exception e) { | ||
| LOG.error("Failed to fetch group from Enclave with error " + e.getMessage()); |
There was a problem hiding this comment.
Don't think there is any need for two log statements here. Could just do LOG.error("Failed to fetch group from Enclave", e) and let log4j log the exception nicely?
| body = RequestBody.create(mediaType, objectMapper.writeValueAsString(content)); | ||
| } catch (final JsonProcessingException e) { | ||
| final String errorMessage = "Failed to serialise request to json body."; | ||
| LOG.error(errorMessage); |
There was a problem hiding this comment.
No need log and throw the exception
| @@ -107,7 +107,7 @@ private PrivateTransactionProcessor mockPrivateTxProcessor() { | |||
|
|
|||
| private Enclave brokenMockEnclave() throws Exception { | |||
| Enclave mockEnclave = mock(Enclave.class); | |||
There was a problem hiding this comment.
throws Exception shouldn't be needed now it's throwing the runtime EnclaveException
| } | ||
|
|
||
| public long determineNonce( | ||
| final String privateFrom, final String[] privateFor, final Address address) { |
There was a problem hiding this comment.
nit: Think it would have been nice if the Enclave provided a findEeaPrivacyGroup(privateFor, privateFrom) method. Probably not part of this PR though
There was a problem hiding this comment.
Yup - this should probably be Orion.
| return new JsonRpcSuccessResponse(request.getId(), Quantity.create(nonce)); | ||
| } catch (final Exception e) { | ||
| LOG.error(e.getMessage(), e); | ||
| return new JsonRpcErrorResponse(request.getId(), JsonRpcError.FIND_PRIVACY_GROUP_ERROR); |
There was a problem hiding this comment.
this looks like the wrong error type
| final String errorMessage = | ||
| String.format( | ||
| "Found invalid number of privacy groups (%d), expected 1.", legacyGroups.size()); | ||
| LOG.error(errorMessage); |
There was a problem hiding this comment.
no need to log and throw, perhaps just throw with the error message. Will be logged in rpc layer
Exposers a Json Rpc which allows "legacy" users to query for the nonce of a given address, in a group of private users.
Exposers a Json Rpc which allows "legacy" users to query for the
nonce of a given address, in a group of private users.