Add rpc filter methods #218
Conversation
| case e: JString => | ||
| extractBytes(e).toOption.getOrElse(throw new RuntimeException(s"Unable to parse topics, expected byte data but got ${e.values}")) | ||
| case other => | ||
| throw new RuntimeException(s"Unable to parse topics, expected byte data but got: $other") |
There was a problem hiding this comment.
oh...so you noticed that :P fixed
| } | ||
| .recover { case ex => errorResponse(rpcReq, InternalError) } | ||
| .recover { case ex => | ||
| log.error("Failed to handle RPC request", ex) |
There was a problem hiding this comment.
👍
We generally miss logging in this area. Would be really helpful to know which methods are being called from Ethereum Wallet.
|
|
||
| val maxBlockHashesChanges = 256 | ||
|
|
||
| var filters: Seq[Filter] = Nil |
There was a problem hiding this comment.
👍
And do you think it's worthy to join filters and lastCheckBlocks within the same Map?
| sender() ! NewFilterResponse(filter.id) | ||
| } | ||
|
|
||
| private def uninstallFilter(id: BigInt): Unit = { |
There was a problem hiding this comment.
The definition states: Additonally Filters timeout when they aren't requested with eth_getFilterChanges for a period of time. ... is that done? If so please point me out where is it and discard this comment 😄
There was a problem hiding this comment.
oops, missed it
| (pendingTransactionsManager ? PendingTransactionsManager.GetPendingTransactions) | ||
| .mapTo[PendingTransactionsManager.PendingTransactions] | ||
| .flatMap { case PendingTransactionsManager.PendingTransactions(pendingTransactions) => | ||
| keyStore.listAccounts() match { |
There was a problem hiding this comment.
Are you sure this filter is required?
There was a problem hiding this comment.
According to documentation no, but geth implementation only returns pending transactions of accounts managed by this node so I think it's needed to stay consistent
| sealed trait Filter { | ||
| def id: BigInt | ||
| } | ||
| case class LogFilter( |
There was a problem hiding this comment.
Is it possible the following field is missing?
removed: TAG - true when the log was removed, due to a chain reorganization. false if its a valid log.
There was a problem hiding this comment.
It is, according to documentation, but...:
a) geth does not return such field
b) I don't think there's a case when this should be set to false (we never return logs from dropped blocks)
There was a problem hiding this comment.
Discard the comment then
| } | ||
|
|
||
| private def getBlockHashesAfter(blockNumber: BigInt): Seq[ByteString] = { | ||
| val bestBlock = appStateStorage.getBestBlockNumber() |
There was a problem hiding this comment.
As this is being queried here https://github.com/input-output-hk/etc-client/pull/218/files#diff-b5ee4e4c8bcd462269de630eb5466a06R127 maybe we can send it as paramenter instead of getting that value again
| recur(blockNumber + 1, Nil) | ||
| } | ||
|
|
||
| private def getPendingTransactions(): Future[Seq[SignedTransaction]] = { |
There was a problem hiding this comment.
It seems this one doesn't work as parity does. Our implementation is returning all the pending transactions every time I query them. In parity, it only returns the ones It hasn't returned before.
This doc statement is confusing, but might be the reason Creates a filter in the node, to notify when new pending transactions arrive . Maybe new is interpreted as parity does, ie: new in terms of this API
| transactionHash: Option[ByteString], | ||
| blockHash: ByteString, | ||
| blockNumber: BigInt, | ||
| address: Address, |
There was a problem hiding this comment.
since we are using Extraction.decompose should we use ByteString for address?
There was a problem hiding this comment.
I've added custom serializer for Address
AlanVerbner
left a comment
AlanVerbner
left a comment
There was a problem hiding this comment.
@LukasGasior1 I've been testing and the code works really good! 🚀
The pending block isn't yet done so i'm skipping tests on that subject
|
|
||
| var filterTimeouts: Map[BigInt, Cancellable] = Map.empty | ||
|
|
||
| implicit val timeout = Timeout(5.seconds) |
There was a problem hiding this comment.
What is this used for? Should it be configurable?
There was a problem hiding this comment.
of course it should
4 participants
Uh oh!
There was an error while loading. Please reload this page.