Add IdentityMetadata to Contractkit #1126

nambrot · 2019-09-27T12:43:11Z

Description

This PR adds the ability for ContractKit consumers to use MetadataManager to fetch metadata as per celo-org/celo-proposals#4, validate it and find claims for an attestation service in the data. It also adds a simple cli command to
do it end-to-end including fetching the metadata URL from the Attestations contract.

Tested

Added a metadata URL on integration, and used the CLI to show me the information in it.

Other changes

This PR adds the packages io-ts and fp-ts as a first proof-of-concept.

io-ts allows us to specify the type of external data that our code expects and gives us runtime validation of that data. Conventionally, we'd have to write extra functions asserting the shape of the data. In types.ts you can see an example of how I specified the structure of the metadata payload, and then simply using IdentityMetadata.decode, io-ts will do all the heavy lifting for us. I expect this to be also handy when building HTTP servers to assert request payloads (+other use cases)

fp-ts is used by io-ts and allows us to specify an alternative way to express computation, especially computation that can fail. Instead of errors that are being thrown, caught and then re-thrown by using try-catch everywhere, one can write functions that encode their failure mode into its return value with Either. Using something like Either allows us to compose computation that can fail more easily than conventional errors. (an example)

I'm looking for feedback if you think we should use io-ts or fp-ts more broadly. It is clear, that this style of programming is going to be less familiar with most developers and thus is the biggest reason to not adopt these packages. However, in my opinion they come with serious benefits to reliability and composition.

Related issues

Addresses Users SBAT handle identity metadata #1104

codecov · 2019-09-27T14:01:25Z

Codecov Report

Merging #1126 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1126   +/-   ##
======================================
  Coverage    66.7%   66.7%           
======================================
  Files         259     259           
  Lines        7454    7454           
  Branches      432     498   +66     
======================================
  Hits         4972    4972           
+ Misses       2386    2384    -2     
- Partials       96      98    +2

Flag	Coverage Δ
#mobile	`66.7% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
packages/mobile/src/utils/formatting.ts	`87.5% <0%> (ø)`	⬆️
packages/mobile/src/identity/reducer.ts	`41.66% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41103b2...d3771c6. Read the comment docs.

jmrossy · 2019-09-27T14:28:14Z

packages/cli/package.json

@@ -62,6 +62,7 @@
    "@types/node": "^10",
    "@types/web3": "^1.0.18",
    "globby": "^8",
+    "prettier": "1.13.5",


I believe this shouldn't be necessary since it's already in the root package json.
Ditto for the typescript line just below it. @mcortesi can you confirm? I believe you had a PR to remove these from the package.json files a while back?

that's right, you don't need that. It's on the root pkg

When I ran yarn run docs, it complained that prettier was an unknown command, so I had to add it to run it

mcortesi

Left some comments

mcortesi · 2019-09-27T14:44:34Z

packages/cli/package.json

@@ -62,6 +62,7 @@
    "@types/node": "^10",
    "@types/web3": "^1.0.18",
    "globby": "^8",
+    "prettier": "1.13.5",


that's right, you don't need that. It's on the root pkg

mcortesi · 2019-09-27T14:45:43Z

packages/contractkit/package.json

@@ -39,7 +42,8 @@
    "@celo/ganache-cli": "git+https://github.com/celo-org/ganache-cli.git#98ad2ba",
    "@celo/protocol": "1.0.0",
    "@types/debug": "^4.1.5",
-    "@types/web3": "^1.0.18"
+    "@types/web3": "^1.0.18",
+    "ts-node": "8.3.0"


this is on the root pkg, shouldn't be here

When running yarn run ts-node from packages/contractkit, my local env complains that ts-node is an unknown command, sounds like the same issue as above?

maybe it's a yarn version problem? do a yarn install on root?
I do it all the time and i don't have that problem

were you abel to fix this after upgrading yarn?

mcortesi · 2019-09-27T15:07:51Z

packages/contractkit/tslint.json

@@ -2,6 +2,8 @@
  "extends": ["@celo/typescript/tslint.json"],
  "rules": {
    "no-global-arrow-functions": false,
-    "no-console": false
+    "no-console": false,


i think it's better to disable this by file or by line, not for the whole project.

mcortesi · 2019-09-27T15:11:17Z

packages/contractkit/src/utils/fp-ts.ts

+import { TaskEither, tryCatch } from 'fp-ts/lib/TaskEither'
+import { writeFileSync } from 'fs'
+
+export class NetworkError extends Error {}


we need to check and have a common way of creating custom erorrs...
I know this is wrong... but i always have to google for it
Check: https://www.google.com/search?q=typescript+subclass+error

mcortesi · 2019-09-27T15:12:32Z

packages/contractkit/src/identity/types.ts

+export type NameClaim = t.TypeOf<typeof NameClaimType>
+export type IdentityMetadata = t.TypeOf<typeof IdentityMetadataType>
+
+export class ValidationError extends Error {}


check error subclassing in js/ts.

Interesting, I was not aware. What would you recommend us to do? https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-extending-built-ins-like-error-array-and-map-work ?

Yeah it's one of the common js weird things.
The link you've sent has a good recommendation.
Also, I remember doing some kind of createError() function to define new error classes

mcortesi · 2019-09-27T16:02:36Z

packages/contractkit/src/identity/types.ts

+
+export const parseMetadata = (str: string) => {
+  const firstPass = parseJSON(str, toError)
+  return chain(deserializePayloads)(firstPass)


it seems to me that we are parsingJSON, and then we assume it has some schema.

In particual, we assume data.claims

Isn't io-ts supposed to do the validation for use?

io-ts can validate a JS object to be of a particular shape, but unless you produce specific types that parse from a string, io-ts does not by default do the JSON parsing itself. So in practice, we would generally do parseMetadata in conjunction with validateMetadata

Yes, but you are iterating it (on deserializePayloads), asssuming that (a) is an object, (b) has a claims property which is an array.

Validating that should be what the validator library does (either io-ts or any other json validator library)

Oh yeah, you are right. Right now this code just assumes an object that has the claims property. It is because the payload itself is serialized twice, which does not fit well into the built-in types (plus eventually verifying the signatures). It seems to me that instead of shoe-horning it into the types, I would actually keep this separate, what do you think?

mcortesi · 2019-09-27T16:03:45Z

packages/contractkit/src/identity/metadata-manager.ts

+export type MetadataError = NetworkError | ValidationError
+
+export class MetadataManager {
+  emptyIdentityMetadata: IdentityMetadata = {


should this be readonly? freezed?

mcortesi · 2019-09-27T16:05:52Z

packages/contractkit/src/identity/metadata-manager.ts

+    return pipe(
+      data,
+      findClaim(ClaimTypes.ATTESTATION_SERVICE_URL),
+      // @ts-ignore Typescript can't infer that the claim must be an Attestation service claim


it can, the problem is in findClaim ;)

I'm taking suggestions on how to improve findClaim. I tried, but couldn't figure out an implementation that correctly types the returned claim unfortunately. I maybe naively blame the compiler

mcortesi · 2019-09-27T16:08:58Z

packages/contractkit/src/identity/metadata-manager.ts

+    )
+
+  findAttestationServiceClaimE = (data: IdentityMetadata): Either<ClaimNotFoundError, string> => {
+    return pipe(


to be honest...
the not so fancy fp impl seems easier to read

findAttestationServiceClaimE = (data: IdentityMetadata) => findClaim(ClaimTypes.ATTESTATION_SERVICE_URL, data).url

findClaim returns an Either which is why you can't just access .url

mcortesi · 2019-09-27T16:11:38Z

packages/cli/src/base.ts

@@ -41,6 +43,13 @@ export abstract class BaseCommand extends Command {
    return this._kit
  }

+  get metadataManager() {


i think this should not be accessible from kit, instead have a module elsewhere that you can import from contractKit to use it. Since it's probably not interesting for most of our users.

Same as we did with BlockExplorer

mcortesi

Left a few comments, but merge when you see fit.
it feels much simpler...!
there was an extra file before, right?

mcortesi · 2019-10-01T11:45:19Z

packages/contractkit/src/identity/metadata.ts

+  }
+
+  addClaim(claim: Claim) {
+    this.data = { claims: [...this.data.claims, this.signClaim(claim)] }


This seems nor one thing, nor the other. We have a mutable object IdentityMetadataWrapper, but we treat this.data as an immutable one.

You can do:

this.data.claims.push(this.signClaim(claim))

and assume it's mutable and you "own" this.data
or treat everything immutable and then do

addClaim(claim: Claim): IdentityMetadataWrapper { return new IdentityMetadataWrapper({ claims: [...this.data.claims, this.signClaim(claim)] }) }

assuming of course you can create IdentityMetadataWrapper with it (which is another suggestion)

Good point!

I'll do the mutation, but will probably leave addClaim as it is. I don't think chaining claims is going to be that common

mcortesi · 2019-10-01T11:49:23Z

packages/cli/src/commands/identity/get-metadata.ts

+            break
+        }
+
+        console.info(`(claim created ${moment.unix(claim.payload.timestamp).fromNow()})\n`)


this could be a interesting helper to add to the cli helpers. relativeDate() or something

I think using moment is easy enough for now, but we can re-assess in the future

mcortesi · 2019-10-01T11:52:09Z

packages/contractkit/src/identity/metadata.ts

+  }
+
+  constructor(data: IdentityMetadata) {
+    const result = IdentityMetadataType.decode(data)


I would argue that this step is for before we call data an IdentityMetadata...
if we "trust" the types, then data is already an IdentityMetadata.

So, for me, this belongs to the fromRawString function, where we construct an IdentityMetadata from a string.

You are right again!

* master: (31 commits) Upgrade to Node 10 (#1148) [faucet] Add custom metrics (#1143) Add IdentityMetadata to Contractkit (#1126) [Wallet] Local currency v1.1 (#1137) Add attestation-service deploy (#1128) [Wallet] A few docs and build cleanup (#1138) [CircleCI]Add comment on how to fix lint checks (#1134) 2019-09-30 integration deployment (#1149) Update web3 provider to new integration url (#1151) [celotool]Add fast mode to celotool invite (#1135) Revert "Feature/909 proxy delegatecall" (#1146) Use contractkit in notification service (#1118) Feature/909 proxy delegatecall (#1003) integration deployment for 2019-09-29 (#1139) Add instructions for npm publication to tag commit (#1117) Client Logs Data Flow script update (#1055) Deploying latest proxy code in genesis (#1122) Enable floating promises check everywhere (fix issues) (#1115) [cli] Solution for build error contractkit on Linux 19.04 distro (#960) [Wallet] Merge back changes made for mx pilot (#1113) ... # Conflicts: # yarn.lock

* master: (35 commits) [Wallet] Network fee in transaction feed (#1145) New About Page Cover (#905) Upgrade to Node 10 (#1148) [faucet] Add custom metrics (#1143) Add IdentityMetadata to Contractkit (#1126) [Wallet] Local currency v1.1 (#1137) Add attestation-service deploy (#1128) [Wallet] A few docs and build cleanup (#1138) [CircleCI]Add comment on how to fix lint checks (#1134) 2019-09-30 integration deployment (#1149) Update web3 provider to new integration url (#1151) [celotool]Add fast mode to celotool invite (#1135) Revert "Feature/909 proxy delegatecall" (#1146) Use contractkit in notification service (#1118) Feature/909 proxy delegatecall (#1003) integration deployment for 2019-09-29 (#1139) Add instructions for npm publication to tag commit (#1117) Client Logs Data Flow script update (#1055) Deploying latest proxy code in genesis (#1122) Enable floating promises check everywhere (fix issues) (#1115) ... # Conflicts: # packages/web/src/about/About.tsx # packages/web/src/about/images/index.ts # packages/web/static/locales/en/about.json

nambrot added 4 commits September 27, 2019 14:18

Add Metadata fetching to ContractKit

4e8c99f

Remove testnet parameter in MetadataManager

255d9db

Type MetadataManager

2a6a991

Rearrange Metadata types

86f2914

nambrot requested review from asaj and mcortesi as code owners September 27, 2019 12:43

nambrot assigned mcortesi, jmrossy and m-chrzan Sep 27, 2019

nambrot added 2 commits September 27, 2019 15:02

Reference right type

7247d52

Check in docs

8637ee2

jmrossy reviewed Sep 27, 2019

View reviewed changes

mcortesi reviewed Sep 27, 2019

View reviewed changes

mcortesi assigned nambrot and unassigned mcortesi Sep 27, 2019

nambrot added 7 commits September 30, 2019 12:29

Make frozen

1fb71fa

PR comments

c8d3686

More comments

0d58745

Remove reference to MetadataManager

bbf6828

Remove custom errors

f6d0bd2

Remove fp-ts file

9aa26e6

Add metadata registration during contract deploy

66ea4e0

mcortesi approved these changes Oct 1, 2019

View reviewed changes

nambrot changed the title ~~Add MetadataManager to Contractkit~~ Add IdentityMetadata to Contractkit Oct 1, 2019

nambrot added 2 commits October 1, 2019 14:47

PR comments

a68a118

Merge branch 'master' into nambrot/identity-metadata-contractkit

18fb330

nambrot added the automerge Have PR merge automatically when checks pass label Oct 1, 2019

Bump celotool contractkit dependency

d3771c6

ashishb merged commit ae13eb8 into master Oct 1, 2019

ashishb deleted the nambrot/identity-metadata-contractkit branch October 1, 2019 14:23

nambrot mentioned this pull request Oct 4, 2019

Fix integration hardcode #1208

Merged

snyk-bot mentioned this pull request Sep 5, 2021

[Snyk] Fix for 1 vulnerabilities denissnykio/celo-monorepo#635

Open

lili2311 mentioned this pull request Sep 18, 2024

[Snyk-dev] Fix for 9 vulnerabilities lili2311/lerna-celo-monorepo#547

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IdentityMetadata to Contractkit #1126

Add IdentityMetadata to Contractkit #1126

nambrot commented Sep 27, 2019 •

edited

Loading

codecov bot commented Sep 27, 2019 •

edited

Loading

jmrossy Sep 27, 2019

mcortesi Sep 27, 2019

nambrot Sep 27, 2019

mcortesi left a comment

mcortesi Sep 27, 2019

mcortesi Sep 27, 2019

nambrot Sep 30, 2019

mcortesi Sep 30, 2019

mcortesi Oct 1, 2019

mcortesi Sep 27, 2019

mcortesi Sep 27, 2019

mcortesi Sep 27, 2019

nambrot Sep 30, 2019

mcortesi Sep 30, 2019

mcortesi Sep 27, 2019

nambrot Sep 30, 2019

mcortesi Sep 30, 2019

nambrot Sep 30, 2019

mcortesi Sep 27, 2019

mcortesi Sep 27, 2019

nambrot Sep 30, 2019

mcortesi Sep 27, 2019

nambrot Sep 30, 2019

mcortesi Sep 27, 2019

mcortesi left a comment

mcortesi Oct 1, 2019

nambrot Oct 1, 2019

mcortesi Oct 1, 2019

nambrot Oct 1, 2019

mcortesi Oct 1, 2019

nambrot Oct 1, 2019

Add IdentityMetadata to Contractkit #1126

Add IdentityMetadata to Contractkit #1126

Conversation

nambrot commented Sep 27, 2019 • edited Loading

Description

Tested

Other changes

Related issues

codecov bot commented Sep 27, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcortesi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcortesi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nambrot commented Sep 27, 2019 •

edited

Loading

codecov bot commented Sep 27, 2019 •

edited

Loading