Skip to content

[ETCM-126] getRawTransactionBy*#720

Merged
lemastero merged 20 commits intodevelopfrom
ETCM-126-get-raw-transaction
Oct 9, 2020
Merged

[ETCM-126] getRawTransactionBy*#720
lemastero merged 20 commits intodevelopfrom
ETCM-126-get-raw-transaction

Conversation

@lemastero
Copy link
Copy Markdown
Contributor

@lemastero lemastero commented Oct 2, 2020 •
edited
Loading

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

@lemastero lemastero force-pushed the ETCM-126-get-raw-transaction branch from e4a4c4c to 79e3d9b Compare October 5, 2020 10:07
@lemastero lemastero changed the title [ETCM-126] get raw transaction WIP [ETCM-126] get raw transaction Oct 5, 2020
@lemastero lemastero changed the title WIP [ETCM-126] get raw transaction [ETCM-126] getRawTransactionBy* Oct 5, 2020
@lemastero lemastero marked this pull request as ready for review October 5, 2020 17:18
@lemastero lemastero requested review from kapke and mmrozek October 5, 2020 17:19
mmrozek
mmrozek reviewed Oct 6, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mmrozek mmrozek left a comment

Choose a reason for hiding this comment

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

Looks ok. Please add some examples to the insomnia workspace

kapke
kapke reviewed Oct 7, 2020
View reviewed changes
Comment thread src/main/scala/io/iohk/ethereum/jsonrpc/EthJsonMethodsImplicits.scala Outdated
Comment thread src/main/scala/io/iohk/ethereum/jsonrpc/JsonRpcController.scala Outdated
Comment thread src/main/scala/io/iohk/ethereum/jsonrpc/RawTransactionCodec.scala
Comment thread src/test/scala/io/iohk/ethereum/jsonrpc/EthServiceSpec.scala
@lemastero
Copy link
Copy Markdown
Contributor Author

lemastero commented Oct 7, 2020

Please add some examples to the insomnia workspace

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

mmrozek
mmrozek approved these changes Oct 8, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mmrozek mmrozek left a comment

Choose a reason for hiding this comment

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

lgtm!

@lemastero
Copy link
Copy Markdown
Contributor Author

lemastero commented Oct 8, 2020

@kapke @mmrozek please rereview.

kapke
kapke approved these changes Oct 9, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kapke kapke left a comment

Choose a reason for hiding this comment

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

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>

Comment thread src/main/scala/io/iohk/ethereum/jsonrpc/EthService.scala
Comment thread src/test/scala/io/iohk/ethereum/jsonrpc/JsonRpcControllerSpec.scala
Some(JInt(1))
)

val response = jsonRpcController.handleRequest(request).futureValue
Copy link
Copy Markdown
Contributor

@kapke kapke Oct 9, 2020

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@lemastero lemastero Oct 9, 2020

Choose a reason for hiding this comment

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

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

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
@lemastero lemastero force-pushed the ETCM-126-get-raw-transaction branch from 8f1bcd9 to 6912d15 Compare October 9, 2020 11:27
@lemastero
Copy link
Copy Markdown
Contributor Author

lemastero commented Oct 9, 2020

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
Copy link
Copy Markdown
Contributor

mmrozek commented Oct 9, 2020

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

@lemastero
Copy link
Copy Markdown
Contributor Author

lemastero commented Oct 9, 2020 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

mmrozek commented Oct 9, 2020

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

@lemastero lemastero merged commit 8a18c76 into develop Oct 9, 2020
@lemastero lemastero deleted the ETCM-126-get-raw-transaction branch October 16, 2020 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@mmrozek mmrozek mmrozek approved these changes

@kapke kapke kapke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lemastero @mmrozek @kapke