[ETCM-126] getRawTransactionBy* #720

lemastero · 2020-10-02T15:45:20Z

Description

Add support for:

Proposed Solution

Important Changes Introduced

HTTP model:
GetRawTransactionByHashRequest
GetRawTransactionByHashResponse
add getRawTransactionByHash in EthService, that reuse logic from existing: getTransactionByHash, extracted common logic to getTransactionDataByHash that returns TransactionData
handle eth_getRawTransactionByHash route JsonRpcController
simplify SignedTransactionEnc
getRawTransactionByBlockHashAndIndexRequest has different implementation than getTransactionByBlockHashAndIndexRequest, I needed signed transaction, please double check this logic

Testing

It looks like it is not possible to write it rpcTest in RpcApiTests there is no getRawTransactionByHash in web3j Ethereum

mmrozek

Looks ok. Please add some examples to the insomnia workspace

src/main/scala/io/iohk/ethereum/jsonrpc/EthJsonMethodsImplicits.scala

src/main/scala/io/iohk/ethereum/jsonrpc/JsonRpcController.scala

src/main/scala/io/iohk/ethereum/jsonrpc/RawTransactionCodec.scala

src/test/scala/io/iohk/ethereum/jsonrpc/EthServiceSpec.scala

lemastero · 2020-10-07T15:19:28Z

Please add some examples to the insomnia workspace

@mmrozek insomnia JSON is updated. Nice catch thank you :)

mmrozek

lgtm!

lemastero · 2020-10-08T20:26:04Z

@kapke @mmrozek please rereview.

kapke

Minor things only. Overall looks good to me!
Please note though, that commit messages are usually prefixed with jira ticket, so they should be like [ETCM-126] <the message>

src/main/scala/io/iohk/ethereum/jsonrpc/EthService.scala

kapke · 2020-10-09T10:36:04Z

src/test/scala/io/iohk/ethereum/jsonrpc/JsonRpcControllerSpec.scala

+      Some(JInt(1))
+    )
+
+    val response = jsonRpcController.handleRequest(request).futureValue


there is set of additional matchers for rpc responses in io.iohk.ethereum.jsonrpc.JRCMatchers, it allows to compare result only, so less boilerplate

Custom marchers from JRCMatchers were used to simplify tests: 7bb0d21

…ionByBlockHashAndIndex

…urning None

…exRequest

…ctions

…ransactionXXX requests

…o ByteString to EthJsonMethodsImplicits from EthService

…ionByBlockNumberAndIndex

…ndex

lemastero · 2020-10-09T11:30:07Z

commit messages are usually prefixed with jira ticket, so they should be like [ETCM-126] <the message>

I like that convention 👍 All commit were reworded to have [ETCM-126] prefix: https://github.com/input-output-hk/mantis/pull/720/commits

mmrozek · 2020-10-09T11:46:41Z

If you renamed all commits you could squash all commit to one as well ;)

Signed-off-by: lemastero <piotr.paradzinski@iohk.io>

lemastero · 2020-10-09T13:52:20Z

If you renamed all commits you could squash all commit to one as well

@mmrozek How about making a squash on merge into develop branch?

Pros:

PR's have a nice history of changes.
PR can be easily reverted from develop if needed
it is easier to make release notes etc

Cons:

if you have PR's depending on other PR's it is a bit harder to update them (but the same problem exists when one do rebase)

mmrozek · 2020-10-09T14:55:57Z

I prefer to use squash commits to merge. Especially in cases like this one, when you have a linear history

lemastero force-pushed the ETCM-126-get-raw-transaction branch from e4a4c4c to 79e3d9b Compare October 5, 2020 10:07

lemastero changed the title ~~[ETCM-126] get raw transaction~~ WIP [ETCM-126] get raw transaction Oct 5, 2020

lemastero changed the title ~~WIP [ETCM-126] get raw transaction~~ [ETCM-126] getRawTransactionBy* Oct 5, 2020

lemastero marked this pull request as ready for review October 5, 2020 17:18

lemastero requested review from mmrozek and kapke October 5, 2020 17:19

mmrozek reviewed Oct 6, 2020

View reviewed changes

kapke reviewed Oct 7, 2020

View reviewed changes

mmrozek approved these changes Oct 8, 2020

View reviewed changes

kapke approved these changes Oct 9, 2020

View reviewed changes

mmrozek approved these changes Oct 9, 2020

View reviewed changes

lemastero added 17 commits October 9, 2020 13:25

[ETCM-126] implement JSON-RPC eth_getRawTransactionByHash

75335b0

[ETCM-126] add eth_getRawTransactionByBlockHashAndIndex

1471d1a

[ETCM-126] add TODO for tests getRawTransactionByHash, getRawTransact…

adde5eb

…ionByBlockHashAndIndex

[ETCM-126] add tests for eth_getRawTransactionByBlockHashAndIndex ret…

5ac3f7a

…urning None

[ETCM-126] add unit tests for eth_getRawTransactionByHash

158e117

[ETCM-126] implement JSON-RPC eth_getRawTransactionByBlockHashAndIndex

64f34b4

[ETCM-126] extract common utils for handle RawTransaction

f99019e

[ETCM-126] extract drop unused JsonRpcController specs

f0e2b72

[ETCM-126] extract common part: getTransactionDataByBlockNumberAndInd…

dd10538

…exRequest

[ETCM-126] extract common part: getTransactionByBlockHashAndIndexRequest

74ce39e

[ETCM-126] review: remove redundant XXXRequest in get transaction fun…

bd63640

…ctions

[ETCM-126] review: remove redundant XXXRequest in get transaction fun…

e1d8d75

…ctions

[ETCM-126] review: unify GetRawXXX responses into RawTransactionResponse

1bbf078

[ETCM-126] review: reuse GetTransactionXXX requests to handle GetRawT…

3061304

…ransactionXXX requests

[ETCM-126] review: remove rawTransactionFromBlock and move encoding t…

d208983

…o ByteString to EthJsonMethodsImplicits from EthService

[ETCM-126] review: add JsonRpcController tests for eth_getRawTransact…

c88e90b

…ionByBlockNumberAndIndex

[ETCM-126] add JSON RPC test for eth_getRawTransactionByBlockHashAndI…

255aa3c

…ndex

[ETCM-126] add JSON RPC test for eth_getRawTransactionByHash

6912d15

lemastero force-pushed the ETCM-126-get-raw-transaction branch from 8f1bcd9 to 6912d15 Compare October 9, 2020 11:27

[ETCM-126] review: use custom matchers from JRCMatchers

7bb0d21

Signed-off-by: lemastero <piotr.paradzinski@iohk.io>

Merge branch 'develop' into ETCM-126-get-raw-transaction

f5fd099

lemastero merged commit 8a18c76 into develop Oct 9, 2020

lemastero deleted the ETCM-126-get-raw-transaction branch October 16, 2020 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-126] getRawTransactionBy* #720

[ETCM-126] getRawTransactionBy* #720

lemastero commented Oct 2, 2020 •

edited

Loading

mmrozek left a comment

lemastero commented Oct 7, 2020

mmrozek left a comment

lemastero commented Oct 8, 2020

kapke left a comment

kapke Oct 9, 2020

lemastero Oct 9, 2020

lemastero commented Oct 9, 2020

mmrozek commented Oct 9, 2020

lemastero commented Oct 9, 2020 •

edited

Loading

mmrozek commented Oct 9, 2020

[ETCM-126] getRawTransactionBy* #720

[ETCM-126] getRawTransactionBy* #720

Conversation

lemastero commented Oct 2, 2020 • edited Loading

Description

Proposed Solution

Important Changes Introduced

Testing

mmrozek left a comment

Choose a reason for hiding this comment

lemastero commented Oct 7, 2020

mmrozek left a comment

Choose a reason for hiding this comment

lemastero commented Oct 8, 2020

kapke left a comment

Choose a reason for hiding this comment

kapke Oct 9, 2020

Choose a reason for hiding this comment

lemastero Oct 9, 2020

Choose a reason for hiding this comment

lemastero commented Oct 9, 2020

mmrozek commented Oct 9, 2020

lemastero commented Oct 9, 2020 • edited Loading

mmrozek commented Oct 9, 2020

lemastero commented Oct 2, 2020 •

edited

Loading

lemastero commented Oct 9, 2020 •

edited

Loading