Refactor CBD API #231

piotr-roslaniec · 2023-06-28T11:56:06Z

Type of PR:

Refactor

Required reviews:

2

What this does:

Replaces Configuration type with a raw porterUri
Replaces defaultConfiguration with getPorterUri based on NuCypher network and not a chainId
Moves Porter out of characters and renames it into PorterClient
Removes porterUri and web3Provider from character constructors and moves them into verbs

Issues fixed/closed:

Related to Alter PRE-adapted tDec API design for CBD-DKG #166
- Alter PRE-adapted tDec API design for CBD-DKG #166 (comment)
- Alter PRE-adapted tDec API design for CBD-DKG #166 (comment)

Why it's needed:

One of the changes necessary to transition into the new CBD API

Notes for reviewers:

There are breaking changes in the API

derekpierre · 2023-06-29T15:33:28Z

src/characters/bob.ts

-  constructor(config: Configuration, secretKey: SecretKey) {
-    this.porter = new Porter(config.porterUri);
+  constructor(porterUri: string, secretKey: SecretKey) {
+    this.porter = new PorterClient(porterUri);


I don't think Bob should take a Porter uri either - just like Alice.

✔️

What do you think about other characters like Decrypter and Encrypter? They are meant to be persisted and transferred so it may make sense to store porterUri instead of using it ad-hoc.

Seems like not. Much like Bob/Alice they use a Porter and therefore should probably take a porterUri as a parameter to functions that perform network actions (eg. retrieve).

wdyt, since you are closer to the code than I am?

I think it's going to add a little bit of bloat to method signatures, but it may be a good trade-off between ergonomics and clarity on what consists of a protocol object.

derekpierre · 2023-06-29T15:35:08Z

src/characters/pre-recipient.ts

-import { Porter } from './porter';
-
-export type PreTDecDecrypterJSON = {
+export type PreDecrypterJSON = {
  porterUri: string;
  policyEncryptingKeyBytes: Uint8Array;
  encryptedTreasureMapBytes: Uint8Array;
  publisherVerifyingKeyBytes: Uint8Array;
  bobSecretKeyBytes: Uint8Array;


If this is no longer PreTDecDecrypter, and just PreDecrypter, then bob's secret keys should no longer be needed, right?

How does the PreDecrypter decrypt without a secret key?

For PRE-tDEC Bob's secret key bytes were publicly known anyway so I thought that's why they were passed in here i.e. the Universal Bob concept.

However, for just PRE, Bob's secret bytes should continue to be a secret to everyone except for Bob. So a few things stand out:

Should a PREDecrypter be handling Bob's secret bytes at all - that seems like a security hole. More philosophically, what is a "decrypter" in the context of PRE? For example, does a PRE decrypter return re-encrypted bytes (encrypted for Bob) that Bob can then decrypt himself instead? Basically it gets the data re-encrypted and is separated from decryption. It's kind of like the line between Porter and Bob or in Python the line between PRERetrievalClient and Bob. In which case, it's not a "decrypter" per se.

If PREDecrypters are persistable they should not be persisting bob's secret bytes. Bob should maintain control of his own secret key, which brings me back to point 1) above.

I might need to go back and look at the existing code, maybe I'm just missing something - I think this is where the API becomes very tricky 😅

I think I have more clarity now on where to go from there: It feels to me like PREDecrypter is no longer a satisfactory abstraction for decryption, and that job should be delegated to Bob. And in that case, maybe we should also revisit the shape and function of PreStrategy.

These items look like a big job, and I'd rather sketch a refactoring in a separate issue (#166), discuss it, and then fix it in another PR.

src/config.ts

src/dkg.ts

derekpierre · 2023-06-29T15:54:39Z

src/sdk/strategy/cbd-strategy.ts

@@ -66,18 +62,26 @@ export class CbdStrategy {

 export class DeployedCbdStrategy {
  private constructor(
-    public readonly decrypter: CbdTDecDecrypter,
+    public readonly decrypter: ThresholdDecrypter,


Ignoring for now, but we need some kind of common naming scheme for the Strategy and its encrypter/decrypter.

DeployedCbdStrategy having a ThresholdDecrypter seems a bit disjointed/disconnected - we need to tie the strategy and encrypter/decrypter together through common naming somehow. I know this is a hot-button topic across the team, so no recommendations from my end for this PR, but something we could collectively discuss and address separately.

derekpierre · 2023-06-29T15:57:20Z

src/sdk/strategy/pre-strategy.ts

+
+    const porterUri = this.cohort.porterUri;
+    const alice = Alice.fromSecretKey(this.aliceSecretKey);
+    const bob = new Bob(porterUri, this.bobSecretKey);


Related to a previous comment - only Bob's public key should be needed for PRE, so secret key should not be needed.

I think I'm missing something about this transition that we're going through, i.e. from PreTDecDecrypter to just PreDecrypter. For example, I thought that Bob needs a secret key to decrypt message kits at some point.

manumonti

LGTM! 👌

Just a couple of thoughts/questions.

Also, a reminder that we should reflect this changes in documentation. Not sure if this is a good moment or if it is better to wait until the refactoring process ends.

manumonti · 2023-07-04T13:41:30Z

src/porter.ts

+type Network = 'mainnet' | 'tapir' | 'oryx' | 'lynx';
+
+const PORTER_URIS: Record<Network, string> = {
+  // TODO: Make sure these are correct


What do you mean with this TODO? If the URLs are correct? Can we do this before merging this PR?

I think these are correct, going to remove TODO

manumonti · 2023-07-04T13:55:49Z

test/unit/cbd-strategy.test.ts

@@ -163,14 +163,14 @@ describe('CbdTDecDecrypter', () => {
  it('serializes to a plain object', async () => {


Should we change the name of this test component?

describe('CbdTDecDecrypter', () => {

to

describe('TresholdDecrypter', () => {

github-actions · 2023-07-05T11:53:06Z

Bundled size for the package is listed below:

build/module/types/ethers-contracts/factories: 82.03 KB
build/module/types/ethers-contracts: 152.34 KB
build/module/types: 156.25 KB
build/module/src/conditions/predefined: 19.53 KB
build/module/src/conditions/context: 42.97 KB
build/module/src/conditions/base: 54.69 KB
build/module/src/conditions: 156.25 KB
build/module/src/kits: 19.53 KB
build/module/src/agents: 35.16 KB
build/module/src/characters: 74.22 KB
build/module/src/policies: 19.53 KB
build/module/src/sdk/strategy: 35.16 KB
build/module/src/sdk: 46.88 KB
build/module/src: 425.78 KB
build/module: 636.72 KB
build/main/types/ethers-contracts/factories: 82.03 KB
build/main/types/ethers-contracts: 152.34 KB
build/main/types: 156.25 KB
build/main/src/conditions/predefined: 19.53 KB
build/main/src/conditions/context: 42.97 KB
build/main/src/conditions/base: 54.69 KB
build/main/src/conditions: 156.25 KB
build/main/src/kits: 19.53 KB
build/main/src/agents: 35.16 KB
build/main/src/characters: 74.22 KB
build/main/src/policies: 19.53 KB
build/main/src/sdk/strategy: 35.16 KB
build/main/src/sdk: 46.88 KB
build/main/src: 429.69 KB
build/main: 640.63 KB
build: 1.25 MB

codecov-commenter · 2023-07-05T11:53:35Z

Codecov Report

Merging #231 (274b73a) into tdec-epic (32e6a94) will decrease coverage by 0.35%.
The diff coverage is 86.00%.

@@              Coverage Diff              @@
##           tdec-epic     #231      +/-   ##
=============================================
- Coverage      83.23%   82.88%   -0.35%     
=============================================
  Files             37       36       -1     
  Lines            978      976       -2     
  Branches         123      109      -14     
=============================================
- Hits             814      809       -5     
- Misses           158      162       +4     
+ Partials           6        5       -1

Impacted Files	Coverage Δ
src/dkg.ts	`51.16% <20.00%> (+1.16%)`	⬆️
src/sdk/strategy/cbd-strategy.ts	`91.48% <60.00%> (-8.52%)`	⬇️
src/policies/policy.ts	`85.71% <75.00%> (ø)`
src/sdk/cohort.ts	`92.30% <81.81%> (+0.30%)`	⬆️
src/porter.ts	`39.02% <87.50%> (ø)`
src/sdk/strategy/pre-strategy.ts	`98.55% <92.85%> (-1.45%)`	⬇️
src/characters/alice.ts	`91.11% <100.00%> (+0.20%)`	⬆️
src/characters/bob.ts	`80.35% <100.00%> (ø)`
src/characters/cbd-recipient.ts	`93.22% <100.00%> (ø)`
src/characters/pre-recipient.ts	`83.07% <100.00%> (-0.99%)`	⬇️
... and 2 more

... and 1 file with indirect coverage changes

github-actions · 2023-08-02T08:58:33Z

Bundled size for the package is listed below:

build/module/types/ethers-contracts/factories: 82.03 KB
build/module/types/ethers-contracts: 152.34 KB
build/module/types: 156.25 KB
build/module/src/policies: 19.53 KB
build/module/src/characters: 78.13 KB
build/module/src/agents: 39.06 KB
build/module/src/sdk/strategy: 31.25 KB
build/module/src/sdk: 42.97 KB
build/module/src/conditions/predefined: 19.53 KB
build/module/src/conditions/context: 42.97 KB
build/module/src/conditions/base: 54.69 KB
build/module/src/conditions: 156.25 KB
build/module/src/kits: 19.53 KB
build/module/src: 437.50 KB
build/module: 648.44 KB
build/main/types/ethers-contracts/factories: 82.03 KB
build/main/types/ethers-contracts: 152.34 KB
build/main/types: 156.25 KB
build/main/src/policies: 19.53 KB
build/main/src/characters: 78.13 KB
build/main/src/agents: 39.06 KB
build/main/src/sdk/strategy: 35.16 KB
build/main/src/sdk: 46.88 KB
build/main/src/conditions/predefined: 19.53 KB
build/main/src/conditions/context: 42.97 KB
build/main/src/conditions/base: 54.69 KB
build/main/src/conditions: 156.25 KB
build/main/src/kits: 19.53 KB
build/main/src: 445.31 KB
build/main: 656.25 KB
build: 1.28 MB

piotr-roslaniec changed the title ~~use ethers abi parser to validate function abi~~ Refactor CBD API Jun 28, 2023

Base automatically changed from validate-abi-w-ethers to tdec-epic June 28, 2023 17:07

piotr-roslaniec mentioned this pull request Jun 29, 2023

Alter PRE-adapted tDec API design for CBD-DKG #166

Closed

piotr-roslaniec marked this pull request as ready for review June 29, 2023 11:13

piotr-roslaniec requested review from KPrasch, theref, derekpierre and manumonti June 29, 2023 11:16

derekpierre requested review from jMyles and cygnusv June 29, 2023 14:14

derekpierre reviewed Jun 29, 2023

View reviewed changes

nucypher deleted a comment from github-actions bot Jun 29, 2023

piotr-roslaniec force-pushed the refactor-api#166 branch from 58824c0 to 982c8f8 Compare June 30, 2023 12:07

nucypher deleted a comment from github-actions bot Jun 30, 2023

manumonti reviewed Jul 4, 2023

View reviewed changes

manumonti approved these changes Jul 5, 2023

View reviewed changes

nucypher deleted a comment from github-actions bot Jul 5, 2023

Base automatically changed from tdec-epic to alpha July 10, 2023 10:39

theref approved these changes Jul 17, 2023

View reviewed changes

piotr-roslaniec added 8 commits August 2, 2023 10:44

simplify cohort-strategy relationship

b3d2f90

feat!: replaced configuration with raw porter uri

fcd7fc0

feat!: porter uris are based on networks, not chain ids

d911481

feat! porter is not a character; renamed to porter client

f975994

feat! remove porter uri and web3 provider from character constructors

a2c1677

apply pr suggestions

69f474f

refactor: simplify equals method in protocol objects

64db30f

apply pr suggestions

6d513bb

fix after rebase

300696b

piotr-roslaniec force-pushed the refactor-api#166 branch from 274b73a to 300696b Compare August 2, 2023 08:57

piotr-roslaniec merged commit c1dfcd2 into alpha Aug 2, 2023
12 checks passed

piotr-roslaniec deleted the refactor-api#166 branch August 2, 2023 09:05

derekpierre pushed a commit to derekpierre/taco-web that referenced this pull request Aug 4, 2023

Refactor CBD API (nucypher#231)

1a14e54

piotr-roslaniec added a commit to piotr-roslaniec/nucypher-ts that referenced this pull request Aug 7, 2023

Refactor CBD API (nucypher#231)

3b95cd1

piotr-roslaniec added a commit to piotr-roslaniec/nucypher-ts that referenced this pull request Aug 7, 2023

Refactor CBD API (nucypher#231)

98ddc12

derekpierre restored the refactor-api#166 branch August 8, 2023 19:38

derekpierre pushed a commit that referenced this pull request Aug 9, 2023

Refactor CBD API (#231)

a3d6295

piotr-roslaniec deleted the refactor-api#166 branch August 16, 2023 08:41

piotr-roslaniec mentioned this pull request Oct 4, 2023

Migrate to a monorepo #297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CBD API #231

Refactor CBD API #231

piotr-roslaniec commented Jun 28, 2023 •

edited

Loading

derekpierre Jun 29, 2023

piotr-roslaniec Jun 29, 2023 •

edited

Loading

derekpierre Jun 30, 2023

piotr-roslaniec Jun 30, 2023 •

edited

Loading

derekpierre Jun 29, 2023

piotr-roslaniec Jun 29, 2023

derekpierre Jun 29, 2023 •

edited

Loading

piotr-roslaniec Jun 30, 2023 •

edited

Loading

derekpierre Jun 29, 2023

derekpierre Jun 29, 2023

piotr-roslaniec Jun 29, 2023

manumonti left a comment

manumonti Jul 4, 2023

piotr-roslaniec Jul 5, 2023

manumonti Jul 4, 2023

piotr-roslaniec Jul 5, 2023

github-actions bot commented Jul 5, 2023

codecov-commenter commented Jul 5, 2023 •

edited

Loading

github-actions bot commented Aug 2, 2023

		@@ -163,14 +163,14 @@ describe('CbdTDecDecrypter', () => {
		it('serializes to a plain object', async () => {

Refactor CBD API #231

Refactor CBD API #231

Conversation

piotr-roslaniec commented Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

piotr-roslaniec Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

piotr-roslaniec Jun 30, 2023 • edited Loading

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

manumonti 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

github-actions bot commented Jul 5, 2023

codecov-commenter commented Jul 5, 2023 • edited Loading

Codecov Report

github-actions bot commented Aug 2, 2023

piotr-roslaniec commented Jun 28, 2023 •

edited

Loading

piotr-roslaniec Jun 29, 2023 •

edited

Loading

piotr-roslaniec Jun 30, 2023 •

edited

Loading

derekpierre Jun 29, 2023 •

edited

Loading

piotr-roslaniec Jun 30, 2023 •

edited

Loading

codecov-commenter commented Jul 5, 2023 •

edited

Loading