KomenciKit #5436

bowd · 2020-10-19T13:36:25Z

Description

KomenciKit is a wrapper for the Komenci service.
It will be consumed by Valora and used during fee-less onboarding.

Other changes

I've extended the BaseWrapper with some convenience attributed:

eventTypes

This will essentially act as an enum of events defined in the contract.
This is useful because we're using the getPastEvents which wasn't strictly typed, it used a string as argument.
And we were using it with strings all over the place which is error prone.
The extension allows the compiler to verify that the event passed in exists on the contract.

methodIds

This is a mapping between method name and method ID. It's used in Komenci to have a more ergonomic way to define meta transaction filters.

codyborn · 2020-10-23T19:21:52Z

packages/komencikit/README.md

+
+## User Guide
+
+To start working with Komencikit you need a to pass in a ContractKit instance, the external account 


nit: incomplete sentence

I drifted off to sleep 😅

packages/komencikit/README.md

codyborn · 2020-10-23T19:47:31Z

packages/komencikit/examples/attestation-flow.ts

+    return
+  }
+
+  const events = await attestations.getPastEvents('AttestationsRequested', {


Can we make these a constant enum defined somewhere?
AttestationsRequested AttestationIssuerSelected WalletDeployed

Yeah it's one thing I wanted to look into. Theoretically they should be autogenerated somewhere. I was expecting to see them on the contract wrapper but either that's not expected or there's something weird going on in the solidity->generated types->contract->wrapper. But I'll look into this some more.

packages/komencikit/package.json

codyborn · 2020-10-23T20:39:56Z

packages/komencikit/src/actions.ts

+
+export const startSession = action<ActionTypes.StartSession, StartSessionPayload, StartSessionResp>(
+  ActionTypes.StartSession,
+  'POST',


Can we define this as an enum type if it doesn't exist already?

packages/komencikit/src/client.spec.ts

packages/komencikit/src/client.ts

codyborn · 2020-10-23T21:25:08Z

packages/komencikit/src/kit.ts

+    while (receipt == null && waited < this.options.txRetryTimeoutMs) {
+      receipt = await this.contractKit.web3.eth.getTransactionReceipt(txHash)
+      if (receipt == null) {
+        await sleep(100)


We can also make this a property under KomenciOptions: txRetryPollingIntervalMs

codyborn · 2020-10-23T22:30:55Z

packages/komencikit/src/kit.ts

+          })
+
+          const deployWalletLog = events.find(
+            (event) => normalizeAddressWith0x(event.returnValues.owner) === this.externalAccount


We should also verify that the implementationAddress in the event matches the one sent to the API. This prevents a compromised Komenci node from deploying a malicious MTWProxy instance and Valora using it as its own.

codyborn · 2020-10-23T22:38:34Z

packages/komencikit/src/kit.ts

+    const attestations = await this.contractKit.contracts.getAttestations()
+
+    const approveTx = await attestations.approveAttestationFee(attestationsRequested)
+    const approveTxSig = await wallet.signMetaTransaction(approveTx.txo, nonce)


nit: it might be better to increment nonce nonce++ here in case we add more txs in the future since we won't need to worry about ordering.

codyborn · 2020-10-23T22:41:24Z

packages/komencikit/src/kit.ts

+      console.debug(`${TAG}/requestAttestations attempt#${attempt} error: `, error)
+    },
+  })
+  public async requestAttestations(


We can add the DEK registration to this API as well.

+1 to DEK registration but think that's now accounted for in the setAccount wrapper. I like that approach because it will allow Valora control over when to submit the transaction

tarikbellamine

Nice work! Left a couple of comments and questions.

One thing that may make sense to think a little about now is analytics. We've got the Valora verification flow pretty well measured at this point and Komenci will be a relative black box. I'm not sure how much tracking makes sense for the first iteration but just wanted to put it on your radar as a means to collect data for debugging and performance measurement.

tarikbellamine · 2020-10-27T21:38:53Z

packages/komencikit/src/login.ts

+import { Address } from '@celo/contractkit'
+import { EIP712TypedData } from '@celo/utils/lib/sign-typed-data-utils'
+
+export const buildLoginTypedData = (


@gnardini is looking into incorporating DeviceCheck and SafetyNet. At least SafetyNet outputs a verifiable token but not sure about DeviceCheck yet. Do we want to accept these as inputs if they are available for the first iteration of Komenci?

Yeah we can add them, not necessarily to the login struct because I think having the captcha token is enough for replay protection. But currently I stripped these from the client under the impression that we'll stick only with captcha. If we feel confident in adding those I can do that but I'd merge this as is in the mean time to have a starting point for the mobile work.

Sounds good. Feel free to merge as is and we can always add those in once we have more clarity.

tarikbellamine · 2020-10-27T21:46:47Z

packages/komencikit/src/verifyWallet.ts

+): Promise<Result<true, WalletValidationError>> => {
+  const code = await contractKit.web3.eth.getCode(walletAddress)
+  // XXX: I'm unsure whether this is safe or if we should store the
+  // bytecode as a constant in `mobile` and pass it into KomenciKit


Based on my convos with @asaj, I think we landed on needing a constant bytecode to be available for comparison to ensure the deployed wallet meets expectations. I can't really speak to what other guardrails might be in place though.

Yeah I guess that was more about how to track the implementation's bytecode itself. We were between using bytecode digests or the actual address of the contract. I was inclining towards bytecode digest because I was under the wrong impression that it would be constant across networks and be easier to manage on Valora - it wouldn't need a different mapping per network. But realistically because we're using library linking it's gonna have different bytecodes therefore we're ok with Valora referring to a MTW implementation by the address, and Komenci verifying server side that address maps to a known implementation.

Here I'm actually verifying the Proxy contract bytecode to ensure that the Proxy itself is what we expect. I suspect we won't need to change the Proxy anytime soon so I think it's safe to extract that from the compilation output. But I'll ping the CAP team to figure out if there are holes in this approach.

tarikbellamine · 2020-10-27T22:10:06Z

packages/komencikit/src/kit.ts

+   */
+  getDistributedBlindedPepper = async (
+    e164Number: string,
+    clientVersion: string


What's clientVersion and how will Valora know it?

@codyborn, keep me honest but AFAIK client version is the actual version of Valora, and it's something that Odis requires for tracking I guess. It's currently passed in as DeviceInfo.getVersion() in identity/privateHashing.ts.

That's right. It's purely for monitoring and lets us know on the ODIS side how many users are still using a given version. This can be helpful when deprecating old code paths.

tarikbellamine · 2020-10-27T22:18:43Z

packages/komencikit/src/kit.ts

+   * pointing it to the implementation passed as an argument
+   * The function takes care of waiting for retrying, waiting for receipt and log parsing
+   *
+   * @param implementationAddress the implementation address Valora requires


Nit: I think the description would be more clear if you referenced the fact that this will be Valora's EOA address

But it's not - but that's also more to your point 😅
This is the address of the MetaTransactionWallet implementation that Valora will have saved as a constant.
Komenci (api) will verify that the implementation is a known implementation that we deployed, and will then deploy a Proxy and set the implementation of that proxy to this address.
Valora then needs to verify:

That it's MTW is actually a Proxy (by verifying the bytecode)

That it's pointing to the chosen implementation

That Valora's EOA is the signer

That the MTW is its own Owner

(these are all handled in KomenciKit)

Ah gotcha that makes sense

tarikbellamine · 2020-10-27T22:30:19Z

packages/komencikit/src/kit.ts

+      console.debug(`${TAG}/requestAttestations attempt#${attempt} error: `, error)
+    },
+  })
+  public async requestAttestations(


+1 to DEK registration but think that's now accounted for in the setAccount wrapper. I like that approach because it will allow Valora control over when to submit the transaction

tarikbellamine · 2020-10-27T22:35:18Z

packages/komencikit/src/kit.ts

+    identifier: string,
+    walletAddress: string,
+    attestationsRequested: number
+  ): Promise<Result<TransactionReceipt, FetchError | TxError>> {


It may be worth adding a separate error type to symbolize the "attestation request limit reached" error, which I believe we are initially setting to 10. Perhaps you can add as an error type within KomenciErrorTypes?

Yeah I didn't add that in because I wanted to do it in a separate PR when I implement it on the server side as well.

tarikbellamine · 2020-10-27T22:58:54Z

packages/komencikit/src/kit.ts

+   */
+  _wallet?: MetaTransactionWalletWrapper
+  private async getWallet(address: string): Promise<MetaTransactionWalletWrapper> {
+    if (this._wallet === undefined || this._wallet.address !== address) {


Nit: Consider using optional chaining

tarikbellamine · 2020-10-27T23:10:44Z

packages/komencikit/src/kit.ts

+    const wallet = await this.getWallet(walletAddress)
+    let nonce = await wallet.nonce()
+
+    const approveTx = await attestations.approveAttestationFee(attestationsRequested)


Seems like there's a check you can do beforehand in the event that attestations have been requested but not revealed and save yourself from submitting these transactions. Example of how we are using it in the Valora flow: https://github.com/celo-org/celo-monorepo/blob/master/packages/mobile/src/identity/verification.ts#L481

I think this depends on how we think about KomenciKit. I would leave this logic in Valora and only think of the requestAttestations function here as a drop-in replacement for the transaction that Valora was sending before. When and if to request additional attestations should be up to Valora.

Makes sense, I agree

tarikbellamine · 2020-10-28T01:54:25Z

packages/komencikit/src/kit.ts

+    const accounts = await this.contractKit.contracts.getAccounts()
+    return this.submitMetaTransaction(
+      walletAddress,
+      accounts.setAccount(name, dataEncryptionKey, walletAddress, proofOfPossession).txo


Don't you need to await this method call like you're doing elsewhere?

It depends on the implementation in the wrapper - these calls result in a CeloTransactionObject but some are async, some are not.

tarikbellamine · 2020-10-28T02:03:00Z

packages/komencikit/README.md

+The KomenciKit library is a wrapper for the Komenci Service API, which is used for fee-less onboarding by allowing 
+the consumer to execute fee-less attestations with the help of MetaTransactions.
+
+The main actions that KomenciKit exposes are:


Do we need a wrapper for escrow.withdraw or are you thinking to just submit through the generic function? This that's the only onboarding on-chain transaction not represented in this list.

And in general, must a transaction have a wrapper in KomenciKit in order to be subsidized?

A transaction doesn't need to have a wrapper. I was actually inclined to remove some of the wrappers and rely on the submitMetaTransaction action that takes in an arbitrary MetaTransaction and pushes it.
That will only work, however with transaction that we allow on the API side by filtering on contract address and methodID.

The only transaction that we actually must wrap is the Attestations.request logic because there we're doing the actual subsidy batch and not just executing a simple meta transaction.

codyborn

LGTM

tarikbellamine

Small request regarding terminology and a needed change to the setAccount method.

tarikbellamine · 2020-10-28T20:26:02Z

packages/komencikit/src/kit.ts

+    const accounts = await this.contractKit.contracts.getAccounts()
+    return this.submitMetaTransaction(
+      walletAddress,
+      accounts.setAccount(name, dataEncryptionKey, walletAddress, proofOfPossession).txo


The walletAddress expected by the accounts.setAccount method is actually the EOA not the MTW.

In general, the use of walletAddress in KomenciKit to refer to the MTW is confusing because in a broader context we are trying to move towards a world where accountAddress === MTW proxy address and walletAddress === EOA address. I understand if you want to keep the terminology as is for the rest of Komenci, but I'd suggest changing it in KomenciKit as it will be confusing for future Valora devs

tarikbellamine

Changes look good. Thanks!

Left one small optional request.

tarikbellamine · 2020-10-29T19:01:49Z

packages/komencikit/src/kit.ts

@@ -193,11 +210,11 @@ export class KomenciKit {
  })
  public async requestAttestations(
    identifier: string,
-    walletAddress: string,
+    metaTxWalletAddress: string,


Nit: this is the only transaction wrapper that has identifier as the first argument and the address as the second. Can we swap these to avoid mistakes when calling?

Yeah I actually thought of this as well I'll make all the function take the metaTxWalletAddress as the first argument.

bowd requested review from asaj, cmcewen, gastonponti, mcortesi, nategraf and timmoreton as code owners October 19, 2020 13:36

bowd added 2 commits October 23, 2020 09:01

Skeleton KomenciKit

8e56e98

Tweak requireAttestations mtx construction

0e90d9c

bowd force-pushed the bowd/komencikit branch from aeef8eb to 0e90d9c Compare October 23, 2020 07:02

bowd added 10 commits October 23, 2020 12:13

Add comments

894227e

Adjust interface slightly

33f9fd9

Fix specs

e9a529f

Fix cross-fetch import

45bdd58

Update readme and cleanup

47c0ab4

Slight refactor of actions

b552c08

Modify dependency graph

a92bdb6

Set komencikit dependency

bf9987a

Update dependencies on komencikit in order to support the dep graph

39cb29c

Lint import order

37ffc41

codyborn reviewed Oct 23, 2020

View reviewed changes

packages/komencikit/README.md Outdated Show resolved Hide resolved

codyborn reviewed Oct 23, 2020

View reviewed changes

packages/komencikit/package.json Outdated Show resolved Hide resolved

codyborn reviewed Oct 23, 2020

View reviewed changes

packages/komencikit/src/client.spec.ts Show resolved Hide resolved

codyborn reviewed Oct 23, 2020

View reviewed changes

packages/komencikit/src/client.ts Show resolved Hide resolved

codyborn reviewed Oct 23, 2020

View reviewed changes

bowd and others added 6 commits October 27, 2020 13:57

Fix events typing

0ee7421

Issue with the type lookup table

af3fe5f

Update docs

2e002a8

Add set account to the wrapper API

b175bc3

Further enhance typing with EventsEnum

b5db21d

Merge branch 'master' into bowd/komencikit

c7ca2f4

tarikbellamine reviewed Oct 28, 2020

View reviewed changes

codyborn approved these changes Oct 28, 2020

View reviewed changes

bowd and others added 4 commits October 28, 2020 10:12

Use optional chaning to cleanup

3eb16ab

Update contractkit

82ab319

Merge branch 'master' into bowd/komencikit

43d8523

Update docs

5efb98e

tarikbellamine suggested changes Oct 28, 2020

View reviewed changes

Disambiguate function parameters and add health check

b42f5bf

tarikbellamine approved these changes Oct 29, 2020

View reviewed changes

bowd and others added 3 commits October 30, 2020 09:15

Split approve and request + some small PR feedback tweaks

6c7cc9e

Merge branch 'master' into bowd/komencikit

7c33c49

Merge branch 'master' into bowd/komencikit

8a7a872

bowd merged commit 3e5e838 into master Oct 30, 2020

medhakothari mentioned this pull request Oct 30, 2020

ContractKit v0.4.18 Release #5652

Merged

This was referenced Jun 21, 2022

[Snyk] Fix for 2 vulnerabilities leojoy95/celo-monorepo#472

Open

[Snyk] Security upgrade apollo-server-express from 2.25.4 to 3.0.0 denissnykio/celo-monorepo#830

Open

snyk-bot mentioned this pull request Sep 27, 2022

[Snyk] Fix for 6 vulnerabilities denissnykio/celo-monorepo#857

Open

aaronmgdr deleted the bowd/komencikit branch August 16, 2023 17:40

lili2311 mentioned this pull request Sep 25, 2023

[Snyk-dev] Fix for 3 vulnerabilities lili2311/lerna-celo-monorepo#112

Open

leojoy95 mentioned this pull request Dec 6, 2023

[Snyk] Fix for 11 vulnerabilities leojoy95/celo-monorepo#792

Open

This was referenced Sep 17, 2024

[Snyk-dev] Fix for 63 vulnerabilities lili2311/lerna-celo-monorepo#527

Open

[Snyk-dev] Security upgrade express from 4.16.4 to 4.21.0 lili2311/lerna-celo-monorepo#543

Open

lili2311 mentioned this pull request Oct 12, 2024

[Snyk-dev] Fix for 31 vulnerabilities lili2311/lerna-celo-monorepo#588

Open


		## User Guide

		To start working with Komencikit you need a to pass in a ContractKit instance, the external account

KomenciKit #5436

KomenciKit #5436

Conversation

bowd commented Oct 19, 2020 • edited Loading

Description

Other changes

eventTypes

methodIds

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

tarikbellamine 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

tarikbellamine Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyborn left a comment

Choose a reason for hiding this comment

tarikbellamine left a comment

Choose a reason for hiding this comment

tarikbellamine Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

tarikbellamine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowd commented Oct 19, 2020 •

edited

Loading

tarikbellamine Oct 28, 2020 •

edited

Loading

tarikbellamine Oct 28, 2020 •

edited

Loading