Skip to content

[ETCM-78 added checkpointing JRC#718

Merged
pslaski merged 1 commit intodevelopfrom
etcm-78-checkpointing-jrc
Oct 14, 2020
Merged

[ETCM-78 added checkpointing JRC#718
pslaski merged 1 commit intodevelopfrom
etcm-78-checkpointing-jrc

Conversation

@pslaski
Copy link
Copy Markdown
Contributor

@pslaski pslaski commented Oct 2, 2020

Description

Ported Checkpointing JRC

Important Changes Introduced

  • new checkpointing RPC API
  • new endpoint - checkpointing_getLatestBlock
  • new endpoint checkpointing_pushCheckpoint

Testing

use insomnia endpoints

@pslaski pslaski requested review from ntallar and rtkaczyk October 2, 2020 11:25
@pslaski pslaski force-pushed the etcm-78-checkpointing-jrc branch 2 times, most recently from b107edd to b89ba0e Compare October 2, 2020 11:28
Copy link
Copy Markdown

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

Only minor comments and LGTM!

Comment thread src/main/scala/io/iohk/ethereum/jsonrpc/JsonRpcController.scala Outdated
Comment thread src/test/scala/io/iohk/ethereum/jsonrpc/CheckpointingJRCSpec.scala Outdated
Comment thread src/test/scala/io/iohk/ethereum/jsonrpc/CheckpointingJRCSpec.scala
@pslaski pslaski force-pushed the etcm-78-checkpointing-jrc branch 2 times, most recently from 66a7921 to fe61eb4 Compare October 9, 2020 14:18
Copy link
Copy Markdown

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

Apart from a minor comment, LGTM!

response should haveResult(expectedResult)
}

it should "getLatestBlock in case of blockchain re-org" in new TestSetup {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Isn't this more a test for the CheckpointingServiceSpec?

Copy link
Copy Markdown
Contributor

@rtkaczyk rtkaczyk left a comment

Choose a reason for hiding this comment

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

LGTM, except for Nico's comment about the test

@pslaski pslaski force-pushed the etcm-78-checkpointing-jrc branch from fe61eb4 to 6f36736 Compare October 12, 2020 11:06
@pslaski pslaski force-pushed the etcm-78-checkpointing-jrc branch from 6f36736 to 63dacbf Compare October 13, 2020 13:17
@pslaski pslaski force-pushed the etcm-78-checkpointing-jrc branch from 63dacbf to e6f4714 Compare October 13, 2020 14:07
Copy link
Copy Markdown

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

LGTM!

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP 

@pslaski pslaski merged commit 8ae3eef into develop Oct 14, 2020
@pslaski pslaski deleted the etcm-78-checkpointing-jrc branch October 14, 2020 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants