Skip to content
This repository was archived by the owner on May 19, 2023. It is now read-only.

Web DV Client SDK#11

Merged
ilanolkies merged 20 commits into
masterfrom
dv-client
Nov 20, 2020
Merged

Web DV Client SDK#11
ilanolkies merged 20 commits into
masterfrom
dv-client

Conversation

@javiesses

@javiesses javiesses commented Nov 13, 2020

Copy link
Copy Markdown
Contributor

Closes #3

  • Deleted old version
  • Get content
  • Create content
  • Login process
  • Tests for every written method
  • Fixed typeorm version across the entire repo
  • README brief
  • Swap content
  • Delete content
  • Refresh access token when necessary
  • Login again when refresh token is expired
  • Refactor internal methods

Open work:

@javiesses
javiesses requested a review from ilanolkies November 13, 2020 20:56
@lgtm-com

lgtm-com Bot commented Nov 13, 2020

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 4231f32 into 90fbf8e - view on LGTM.com

new alerts:

  • 1 for Ineffective parameter type

@ilanolkies ilanolkies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First of all,

Overrides to perform operation with multiple values (get content from multiple keys for instance)

do we want to do this or use different method names?

@ilanolkies ilanolkies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some minor changes we talked about:

  • Activate test:ci script to enable Circle CI before merging
  • DID Auth token management is white box. That lead into two conclusions: analyse wether we need to test this or not, and remove access token related methods from client interface.

@javiesses
javiesses force-pushed the dv-client branch 2 times, most recently from f9a16bb to e9bdb0c Compare November 18, 2020 20:10
@javiesses
javiesses marked this pull request as ready for review November 18, 2020 20:53

@ilanolkies ilanolkies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fantastic work!

Comment thread modules/ipfs-cpinner-client/src/auth-manager.ts Outdated
Comment thread modules/ipfs-cpinner-client/src/constants.ts Outdated
Comment thread modules/ipfs-cpinner-client/src/auth-manager.ts
Comment thread modules/ipfs-cpinner-client/src/index.ts
Comment thread modules/ipfs-cpinner-client/README.md Outdated

- Stores the authentication credentials in the given storage

## Usage

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would move all this section directly to docs. It is very big for a readme.

Suggested change
## Usage
## Usage
[Read the docs](https://rsksmart.github.io/rif-identity-docs/data-vault/cpinner/cpinner-client)

I will then review and iterate docs over that PR

@ilanolkies ilanolkies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I missed the option. I now put request changes for stuff described above.

@ilanolkies ilanolkies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fantastic

@ilanolkies
ilanolkies merged commit 44777c3 into master Nov 20, 2020
@delete-merged-branch
delete-merged-branch Bot deleted the dv-client branch November 20, 2020 14:39
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.

Create DataVaultProvider Web Client SDK

2 participants