-
Notifications
You must be signed in to change notification settings - Fork 78
ETS integration RPC endpoints #966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2c8c3ad to
c3ce44b
Compare
…pment, also simplifies dist mapping a bit
Signed-off-by: Igor Grahovac <[email protected]>
…cording to eth spec for retesteth. Fixed genesis loading for regular node
…null fields in JSON response, add coinbase field to BlockResponse
…nature fields required by retesteth
…d storing of contract codes and storage data
2b7537d to
9dcde25
Compare
| { case jv => deserializeUint256String(jv) }, | ||
| PartialFunction.empty | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting seems off here
| JsonRpcControllerMetrics.recordMethodTime(request.method, time) | ||
| } | ||
| } | ||
| .flatTap { response => Task { log.debug(s"sending response ${response.inspect}") } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: find out why there's no tap
| .fold(number => blockchain.getBlockByNumber(number), hash => blockchain.getBlockByHash(hash)) | ||
|
|
||
| if (blockOpt.isEmpty) { | ||
| Task.now(Right(AccountsInRangeResponse(Map(), ByteString(0)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jvdp do you know why this task is here? am I missing a side-effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be a bug and should be returned, I'm pretty sure. It would make the .gets below safe.
dzajkowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the intrusive paths taken (like in BlockPreaprator) but let's get this merged and move forward. Hopefully we can modularise the app and have such modules be testmode specific.
| accountRangeOffset = 0 | ||
|
|
||
| Task.now(Right(SetChainParamsResponse())) | ||
| SetChainParamsResponse().rightNow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being unfamiliar to this I find this worse, somehow... but it's purely subjective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm kinda surprised that the core RPC methods aren't overloaded for 'bare' (non-Task) responses. Should be possible, right?
Description
Initial integration of ETS (Ethereum tests) executed through Retesteth test suite.
Proposed Solution
Some of existing endpoints were updated according to newer changes in the runner and the tests. Additional methods were added, such as test_importRawBlock, test_getLogHash, debug_accountRangeAt and debug_storageRangeAt. Follow up tasks/bugs are created in order to ensure further progress and fixes for the tests that are failing.
Modifications were made to standard eth RPC responses because of Mantis custom fields that are avalable only when "testmode" is configured.