Review of current master state (5f0e8ce)

[devsnd]

This is a review of the Code in 5f0e8ce (since it's to late to create a PR now).

General Nitpicks:

• Currently you're using axios to implement your HTTP interface. On the browser side there is a new standard API for HTTP requests called fetch. I think it would be nice to use that module instead, then you had one dependency less on the browser side. (Node would still need to install fetch-node)

Specific Nitpicks:

• As long as the HTTP interface is still unstable I think we should not make any presumptions about it. AFAIK it does not yet support SSL. I propose to remove the "secure" option from the EpochHttpClient for now.

• EpochHttpClient.post has options.config which are actually the headers of the reqeust. It would be nice if it was named options.headers instead.

• I think it would be better to allow to let the getCommitmentHash parameter salt to be filled automatically. Developers might use it incorrectly and we can prevent that by calculating a hash for them. Also in the readme you state that you would use a 256bit int as salt, but such a number does not exist in plain JS. Everything is a 64bit float (and you have integer precision up to 32 bits). So maybe you can ask the core team if having a 32bit salt is secure enough.

• in waitForBlock the timeout is actually the polling Interval. But maybe a timeout would be a good idea as well.

• it would be nice if made the example.js into the actual test-suite of the thing.

Final Note:

Your code is nice and tidy and easy to understand. I like it. What's especially interesting is that it shows some inconsistencies in the API that we should collect on Thursday.

E.g. the naming of the parameters changes from salt to name_salt. There are multiple inconsistencies like this and your code shows them as you stay consistent.

Also, what I too had to work-around, was the double JSON-encoded pointers which is a bit ugly on the API side.

[tillkolter]

thx for your comments @devsnd

I must confess, that I didn't really take time to consider alternatives to axios, simply because it seemed so common practice to use it after all the code I have been studying. I will consider using fetch standard over axios but, I would like to do some research on it first. Although some posts that I found state that Axios is still having advantages over the standard.

I wanted to add the security parameter from the very beginning, because I think it doesn't do any harm and empowers community developers to secure their potentially public servers as much as they want.

I second your note about the options.config, will change it.

You were right, the swagger documentation is giving the right hint, expecting int64.

I agree that timeout is misleading for an external parameter, although JS standard spec calls the parameter for setInterval exactly like that, so I will refactor to call it intervalTimeout. I am not sure if having a timeout for the global waiting time wouldn't do more harm than it would help.

Integrating the example for the lifecycle to an integration test suite is on my TODO list, but I think we should sync our activities on both language implementations that we can share some general test setup.

[tillkolter]

https://github.com/aeternity/aepp-sdk-js/pull/2