Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Full JSON-RPC docs + sync tests. #4335

Merged
merged 23 commits into from Feb 1, 2017
Merged

Full JSON-RPC docs + sync tests. #4335

merged 23 commits into from Feb 1, 2017

Conversation

maciejhirsz
Copy link
Contributor

Closes #1905.

No docs from Rust code yet, but this PR adds a JS test that checks if all RPC methods defined in traits are added to interfaces/*.js files (from which markdown is generated), and all methods listed in interfaces/*.js exist in Rust source.

Can continue on moving the source of truth away from JS to Rust in #2646.

@maciejhirsz maciejhirsz added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Jan 27, 2017
@@ -48,24 +43,26 @@ function error (log) {
}

function printType (type) {
return type2print.get(type) || type.name;
return type.print || `\`${type.name}\``;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to decide on wether we want template string even if they require escaping `.

@derhuerst
Copy link
Contributor

I know this PR didn't introduce it, but I can see isObject fail easily. Use lodash.isPlainObject.

const sectionA = spec[a].section || '';
const sectionB = spec[b].section || '';

if (sectionA === sectionB) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will produce unexpected results if sectionA.localeCompare(sectionB) === 0.

const flatlist = {};
const allowedTypes = [Array, Boolean, Object, String];

Object.keys(customTypes).map((key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

allowedTypes.concat(Object.values(customTypes))

obj.type === Integer ||
obj.type === Quantity;
});
expect(obj).to.satisfy(() => allowedTypes.indexOf(obj.type) >= 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

allowedTypes.includes

Copy link
Contributor

@ngotchac ngotchac left a comment

Choose a reason for hiding this comment

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

Small comments

obj.type === Integer ||
obj.type === Quantity;
});
expect(obj).to.satisfy(() => allowedTypes.indexOf(obj.type) >= 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use Array::includes method

@@ -76,6 +76,11 @@ export default class Parity {
.execute('parity_dappsInterface');
}

decryptMessage (address, data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to add a quick test for this one.

@@ -292,6 +297,11 @@ export default class Parity {
.then(outAddress);
}

postSign (address, hash) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to add a quick test for this one too.

'signer',
'traces',
'web3'
].map((name) => fs.readFileSync(path.join(traitsDir, `${name}.rs`)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use a glob like pattern here ? Ie. reading all the Rust files in the traits directoy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I had done initially, but some of the traits are either unimplemented or deprecated. That was before I started parsing comments for @deprecated and @unimplemented tags though, can revisit the original idea with that in mind.

rustMethods[group][name] = true;
});

describe('Rust defined JSON-RPC methods', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to follow the test names convention with jsonrpc/interfaces as the top test.

@@ -18,6 +18,7 @@ import { Data } from '../types';

export default {
getHex: {
nodoc: 'Not present in Rust code',
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we need to make a call to remove these or move out of the way. (DB is deprecated anyway.) Should just log it, no need to clutter this one.

(Same probably goes for future work, e.g. shh - which we may have soon anyway)

@@ -180,10 +149,10 @@ The following options are possible for the \`defaultBlock\` parameter:
desc: 'Makes a call or transaction, which won\'t be added to the blockchain and returns the used gas, which can be used for estimating the used gas.',
params: [
{
type: Object,
desc: 'See [eth_call](#eth_call) parameters, expect that all properties are optional.',
type: CallRequest,
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 Great, definately helps. When we have these we are actually moving closer to being able to just auto-detect the input/output formatters, i.e. we are one step closer to having the JS auto-generated.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 31, 2017
@jacogr jacogr merged commit ed09a76 into master Feb 1, 2017
@jacogr jacogr deleted the mh-rpctests branch February 1, 2017 09:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants