-
Notifications
You must be signed in to change notification settings - Fork 60
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
Ensure Schema compliance with execution-apis schema #372
Conversation
Signed-off-by: Anton Rusev <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #372 +/- ##
==========================================
+ Coverage 65.66% 73.34% +7.67%
==========================================
Files 8 8
Lines 798 799 +1
Branches 130 131 +1
==========================================
+ Hits 524 586 +62
+ Misses 237 171 -66
- Partials 37 42 +5
Continue to review full report at Codecov.
|
Signed-off-by: Anton Rusev <[email protected]>
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.
Looking good. Some clarification and improvements
validateResponseSchema(methodsResponseSchema.eth_chainId, response); | ||
}); | ||
|
||
it('should execute "eth_coinbase"', async function () { |
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.
it('should execute "eth_coinbase"', async function () { | |
it('should execute "eth_coinbase"', function () { |
validateResponseSchema(methodsResponseSchema.eth_call, response); | ||
}); | ||
|
||
it('should execute "eth_chainId"', async function () { |
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.
it('should execute "eth_chainId"', async function () { | |
it('should execute "eth_chainId"', function () { |
expect(isValid).to.be.true; | ||
}); | ||
|
||
it('should execute "eth_accounts"', async function () { |
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.
it('should execute "eth_accounts"', async function () { | |
it('should execute "eth_accounts"', function () { |
validateResponseSchema(methodsResponseSchema.net_version, response); | ||
}); | ||
|
||
it('should execute "web3_clientVersion"', async function () { |
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.
it('should execute "web3_clientVersion"', async function () { | |
it('should execute "web3_clientVersion"', function () { |
validateResponseSchema(methodsResponseSchema.net_listening, response); | ||
}); | ||
|
||
it('should execute "net_version"', async function () { |
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.
it('should execute "net_version"', async function () { | |
it('should execute "net_version"', function () { |
validateResponseSchema(methodsResponseSchema.eth_syncing, response); | ||
}); | ||
|
||
it('should execute "net_listening"', async function () { |
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.
it('should execute "net_listening"', async function () { | |
it('should execute "net_listening"', function () { |
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
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.
LG.
Minor suggestion
packages/relay/src/lib/eth.ts
Outdated
@@ -778,13 +778,13 @@ export class EthImpl implements Eth { | |||
input: contractResult.function_parameters, | |||
maxPriorityFeePerGas: maxPriorityFee, | |||
maxFeePerGas: maxFee, | |||
nonce: contractResult.nonce, | |||
nonce: contractResult.nonce === null ? null : EthImpl.numberTo0x(contractResult.nonce), |
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.
nit: Add a nullable param to EthImpl.numberTo0x()
.
With default true and use this logic here and a few places.
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 suggest to create a new static function eg. EthImpl.nullableNumberTo0x
. If we modify numberTo0x
we will need to refactor/touch a lot of rpc methods because they rely on the returned type of numberTo0x
to be a string
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
Signed-off-by: Anton Rusev <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Signed-off-by: Anton Rusev [email protected]
Description:
openrpc.spec.ts
test file to relaygetBlockTransactionCountByHash
andgetBlockTransactionCountByNumber
to return string instead of number to conform the ethereum RPC schemaRelated issue(s):
Fixes #295
Notes for reviewer:
Checklist