Add pending object to GraphQL queries #1419
Conversation
| } | ||
|
|
||
| public Transaction getTransaction() { | ||
| return transaction; | ||
| } | ||
|
|
||
| public long getBlockNumber() { | ||
| public Optional<Long> getBlockNumber() { |
There was a problem hiding this comment.
You could use OptionalLong here
There was a problem hiding this comment.
OptionalLong is not used in this module, so using it in one place has many cascading changes. More suitable for a cleanup PR than a feature PR.
I opened PIE-1563 to clean these up in general.
There was a problem hiding this comment.
One problem is that OptionalLong breaks some Optional.map calls. For example, this won't compile:
public OptionalLong getEstimateGas(final DataFetchingEnvironment environment) {
final Optional<CallResult> result = executeCall(environment);
return result.map(CallResult::getGasUsed);
}
| return blockHash; | ||
| } | ||
|
|
||
| public int getTransactionIndex() { | ||
| public Optional<Integer> getTransactionIndex() { |
There was a problem hiding this comment.
You could use OptionalInt here
There was a problem hiding this comment.
See comment on OptionalLong
| final Address addr = dataFetchingEnvironment.getArgument("address"); | ||
| final Long bn = dataFetchingEnvironment.getArgument("blockNumber"); | ||
| final long latestBn = blockchainQuery.latestBlock().get().getHeader().getNumber(); | ||
| final Optional<WorldState> ows = blockchainQuery.getWorldState(latestBn); |
There was a problem hiding this comment.
(nit) Can we use full names for the vars here? ows -> worldState; bn -> blockNumber, etc
| // until the miner can expose the current "proposed block" we have no | ||
| // speculative environment, so estimate against latest. | ||
| public Optional<CallResult> getCall(final DataFetchingEnvironment environment) { | ||
| return executeCall(environment); |
There was a problem hiding this comment.
Are we executing call twice? Here and in getEstimateGas? If so, anyway to dedupe?
There was a problem hiding this comment.
There are two methods specified in the EIP-1767, one that returns the entire call result and one that wants us to estimate the gas. Executing the call and getting the gasUsed is the easiest way to estimate, so they are deduped by calling executeCall (which is not exposed) and returning what we need. It could be simplified by making getCall be executeCall.
| if (bn != null) { | ||
| blockNumber = bn; | ||
| final Optional<Long> txBlockNumber = transactionWithMetadata.getBlockNumber(); | ||
| final Optional<Long> bn = Optional.ofNullable(environment.getArgument("block")); |
There was a problem hiding this comment.
Is it possible for the block param here to be a hash??
There was a problem hiding this comment.
No, the spec has the block param as a Long representing the block number.
| @@ -0,0 +1,22 @@ | |||
| { | |||
| "request": | |||
| "{ pending { transactionCount transactions { from { address } to { address } } account(address:\"0x6295ee1b4f6dd65047762f924ecd367c17eabf8f\") { balance} estimateGas(data:{}) call (data : {from : \"a94f5374fce5edbc8e2a8697c15331677e6ebf0b\", to: \"0x6295ee1b4f6dd65047762f924ecd367c17eabf8f\", data :\"0x12a7b914\"}){data status}} }", | |||
There was a problem hiding this comment.
(optional) Can we format the request to be more readable?
There was a problem hiding this comment.
It's json, which lacks multiline strings. And the graphql syntax for the call is not json. So no, not really.
| "data": { | ||
| "pending": { | ||
| "transactionCount": 0, | ||
| "transactions": [], |
There was a problem hiding this comment.
Can we add some more tests that return non-empty tx's here?
There was a problem hiding this comment.
mocked in a pending transaction that will return nonce and gas.
PR description
The initial GraphQL bounty did not include the
pendingobject, which was not in the bounty spec. Add this for completeness.Fixed Issue(s)