Skip to content
This repository has been archived by the owner on Feb 1, 2021. It is now read-only.

Add first draft of usage #9

Merged
merged 2 commits into from
May 2, 2019
Merged

Add first draft of usage #9

merged 2 commits into from
May 2, 2019

Conversation

aldigjo
Copy link
Contributor

@aldigjo aldigjo commented Apr 22, 2019

No description provided.

Copy link
Contributor

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Nice initiative to start documentation for this.
There are 2 things that need attention.

  1. RNUportSigner should probably not be promoted and kept only for compatibility
  2. we need to mention the key protection levels and what their behaviors are.

README.md Outdated
//Sign a JWT
const exampleJwtPayload = { iss: this.props.address, aud: this.props.address, name: 'test'}

RNUportSigner.signJwt(this.props.address,
Copy link
Contributor

Choose a reason for hiding this comment

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

this.props.address is highly specific and can induce confusion.
I think address is enough

README.md Outdated

// TODO: What to do with the module?
RNUportSigner;
## Basic Signer Usage
Copy link
Contributor

Choose a reason for hiding this comment

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

The non-hd signer should be marked as deprecated.
The main reason is that there is no way to recover the keys and that may not be obvious.

README.md Outdated
rlpEncodedTx, //RLP Encoded eth transaction
'simple'
).then( txSig => {
console.log(sig.r)
Copy link
Contributor

Choose a reason for hiding this comment

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

the lambda has txSig param but the logs are using sig.r

Copy link
Contributor

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Looks great

@mirceanis mirceanis merged commit 85204bb into master May 2, 2019
@mirceanis mirceanis deleted the update-readme branch May 8, 2019 16:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants