Move Indy NodeJS wrapper codebase here#1
Move Indy NodeJS wrapper codebase here#1ajile-in wants to merge 3 commits intohyperledger-aries:masterfrom
Conversation
Signed-off-by: Ajay Jadhav <jadhavajay@gmail.com>
Signed-off-by: Ajay Jadhav <jadhavajay@gmail.com>
There was a problem hiding this comment.
Jadhavajay, I understand that you are just getting started, but I'd like to understand the goal of this SDK before I review. This is probably better to discuss on a call, but I assume the goal is to implement a higher level agent API.
Since Aries is supposed to allow other implementations other than indy, it would seem reasonable that we at least start by deciding on a directory structure which would allow other implementations to be plugged in.
For example, perhaps something similar to:
-- api (The Aries agent API - to be defined)
-- spi (The service provider API which is used by the Aries agent APIs - i.e. an abstraction of the indy APIs)
-- indy (the current indy code goes here)
-- test (indy test code here)
-- another implementation of the SPI in the future
-- test (test for the higher-level agent APIs)
matt-raffel-kiva
left a comment
matt-raffel-kiva
left a comment
There was a problem hiding this comment.
For all js files: terminate each statement with semicolon. replace var with const and let in functions.
| try { | ||
| this.indyCurrentErrorJson = capi.getCurrentError() | ||
| var details = JSON.parse(this.indyCurrentErrorJson) | ||
| if (typeof details.message === 'string') { |
There was a problem hiding this comment.
var should be replaced with const to avoid potential conflicts in global namespace
| @@ -0,0 +1,80 @@ | |||
| var util = require('util') | |||
There was a problem hiding this comment.
every line should be terminated with semicolon. this is an industry standard and is enforced with linters.
| this.indyBacktrace = details.backtrace | ||
| } | ||
| } catch (e) { | ||
| } |
There was a problem hiding this comment.
we need to do something here. just eating the exception will make it very difficult to debug when a problem happens.
| @@ -0,0 +1,27 @@ | |||
| var IndyError = require('./IndyError') | |||
|
|
|||
There was a problem hiding this comment.
all lines should be terminated with semicolon.
|
|
||
| function wrapIndyCallback (cb, mapResponse) { | ||
| var promise | ||
| if (!cb) { |
There was a problem hiding this comment.
var should be replaced with let
| var tempy = require('tempy') | ||
|
|
||
| test('anoncreds', async function (t) { | ||
| var pool = await initTestPool() |
There was a problem hiding this comment.
local variables need to be declared with const or let.
| @@ -0,0 +1,27 @@ | |||
| var test = require('ava') | |||
There was a problem hiding this comment.
please see my comments for the previous files. same for this one.
| @@ -0,0 +1,68 @@ | |||
| var test = require('ava') | |||
There was a problem hiding this comment.
please see my comments for the previous files. same for this one.
| @@ -0,0 +1,87 @@ | |||
| var test = require('ava') | |||
There was a problem hiding this comment.
please see my comments for the previous files. same for this one.
| @@ -0,0 +1,62 @@ | |||
| var test = require('ava') | |||
There was a problem hiding this comment.
please see my comments for the previous files. same for this one.
|
Thanks @matt-raffel-kiva for your comments on this PR. We (@smithbk & me) discussed last week to have a Zoom call together and discuss the roadmap of how to take this further with Aries Framework idea in mind. We also discussed about using TypeScript instead of ES. I will catch up with you on RocketChat and will schedule a Zoom call. |
|
I was surprised to see this PR closed. I agree with @smithbk that we should define how to be ledger-independent, but I don't think that should prevent taking the existing Indy code as the foundation for an aries-js-wrapper and then iterating. It's hard for interested contributors to know how to collaborate when the repository is completely empty. A simple README.md explaining the current state of the project and current efforts would also help a lot. |
|
I don’t have maintainer rights on that repo. If I could get that, then I could help out more.
… On Sep 11, 2019, at 1:46 PM, Richard Esplin ***@***.***> wrote:
I was surprised to see this PR closed. I agree with @smithbk that we should define how to be ledger-independent, but I don't think that should prevent taking the existing Indy code as the foundation for an aries-js-wrapper and then iterating. It's hard for interested contributors to know how to collaborate when the repository is completely empty. A simple README.md stating the current state of the project and current efforts would also help a lot.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I apologize. I did a force-push to fix a DCO issue, because I didn't see there was a PR in flight. This is my fault. |
|
@esplinr I can't re-open the PR b/c master changed under it. |
|
Thanks for the explanation @ryjones . That makes sense. I misinterpreted what was going on based on the comments, but now I see that @smithbk was only prompting a helpful discussion and a decision hadn't yet been made. @matt-raffel-kiva: I think @jadhavajay can work with @ryjones to grant those rights. |
5 participants
To start with I am moving the Indy NodeJS wrapper as it is here, with no code changes at all except few Readme changes.