Refactor packages APIs #301

piotr-roslaniec · 2023-09-22T15:30:33Z

Type of PR:

Refactor
Other

Required reviews:

2

Issues fixed/closed:

Closes Migrate Node v16.x to v18.x #272
Closes Add a multichain condition example #274
Closes Deprecate PRE-tDec #303

What this does:

Continues refactoring of monorepo packages
Configures tests to use inlined WASM when using @nucypher/nucypher-core
Refactors from character-based to functional API
- In the taco package
- ~~In the pre package~~ Not a priority
Updates taco for compatibility with dkg-alpha-13, including examples
Adds a demos/taco-demo

Notes for reviewers:

Based on Improve DX when working with WASM dependencies #299 - Please review it first
There are some other demos that were not migrated yet, tracked here: Release @nucypher/taco #308

codecov-commenter · 2023-09-22T15:34:22Z

Codecov Report

Merging #301 (0a13562) into monorepo (15ea563) will increase coverage by 22.58%.
Report is 2 commits behind head on monorepo.
The diff coverage is 95.08%.

@@              Coverage Diff              @@
##           monorepo     #301       +/-   ##
=============================================
+ Coverage     65.67%   88.25%   +22.58%     
=============================================
  Files            48       33       -15     
  Lines          5223     2409     -2814     
  Branches         28      205      +177     
=============================================
- Hits           3430     2126     -1304     
+ Misses         1787      254     -1533     
- Partials          6       29       +23

Files	Coverage Δ
packages/pre/src/characters/alice.ts	`87.66% <100.00%> (ø)`
packages/pre/src/characters/bob.ts	`85.61% <100.00%> (ø)`
packages/pre/src/characters/index.ts	`100.00% <ø> (ø)`
packages/pre/src/cohort.ts	`89.28% <100.00%> (ø)`
packages/pre/src/index.ts	`100.00% <100.00%> (ø)`
packages/pre/src/keyring.ts	`93.67% <100.00%> (ø)`
packages/pre/src/kits/index.ts	`100.00% <ø> (ø)`
packages/pre/src/kits/message.ts	`91.25% <100.00%> (ø)`
packages/pre/src/kits/retrieval.ts	`77.27% <100.00%> (ø)`
packages/taco/src/conditions/base/contract.ts	`95.74% <ø> (ø)`
... and 23 more

... and 14 files with indirect coverage changes

cygnusv

First pass. Looking good!

cygnusv · 2023-10-05T09:22:30Z

demos/taco-demo/src/App.tsx

+import {
+  conditions,
+  decrypt,
+  encrypt,
+  getPorterUri,
+  initialize,
+  ThresholdMessageKit,
+} from '@nucypher/taco';


Look how shiny! 🤩

It does look very clean 🎸

examples/README.md

examples/taco/nextjs/src/app/page.tsx

cygnusv · 2023-10-05T10:06:03Z

examples/taco/nodejs/src/index.ts

+
+const privateKey = process.env.PRIVATE_KEY;
+if (!privateKey) {
+  throw new Error('PRIVATE_KEY is not set.');


We have a hardcoded example key for Enrico in nucypher. See https://github.com/nucypher/nucypher/blob/4b978025a262f240a9afb9869257c070a33d767d/tests/constants.py#L88

Maybe we can use this here as a fallback?

On one hand, it makes sense - This Enrico is already configured on testnet (allow-listed and so on) but on the other hand, I'm not sure what happens when a number of users start testing their apps with this default character.

I don't think we have much of a choice for tapir 🤔 , unless:

we make ritual initiation public (for say tapir)

grant initiator roles to those who ask

only specific Enricos would be allowed (global allow list) to encrypt for rituals.

If neither 1 or 2 are feasible, at the very least we can probably provide more than one test authorized Enrico account private key.

Decided to not pursue this change in the PR, tracking it here: #313

cygnusv · 2023-10-05T10:09:53Z

examples/taco/nodejs/src/index.ts

+  );
+
+  console.log('Decrypting message...');
+  const porterUri = getPorterUri('lynx'); // Test network


How about we create some constants somewhere like TacoNetwork.DEV_TESTNET = 'lynx' and TacoNetwork.TESTNET = 'tapir', and we reduce a bit the cognitive load for adopters? After all, how necessary is that adopters know that the testnet is called lynx/tapir?

Thoughts? @piotr-roslaniec @KPrasch @derekpierre

It's another level of abstraction above domains which at this level of public exposure makes sense to me! 👍🏻

I suggest that we adopt the nucypher/nucypher convention to call these "domains" instead of "networks" for accuracy (although imperfect since it's another layer):

TacoDomain.DEV; TacoDomain.TEST

I agree with both suggestions, tracking it in #311

Agreed 👍

examples/taco/webpack-5/src/index.html

cygnusv · 2023-10-05T10:21:28Z

packages/shared/abis/Coordinator.json

@@ -1,1375 +1,1423 @@
 [


Opened #310 to use our NPM package instead of manually tracking here. The NPM package probably needs updating, though (cc @manumonti)

Nice - this thought crossed my mind as well.

cygnusv · 2023-10-05T10:25:23Z

packages/shared/src/contracts/registry.ts

@@ -13,7 +13,7 @@ const POLYGON: Contracts = {

 const MUMBAI: Contracts = {
  SUBSCRIPTION_MANAGER: '0xb9015d7b35ce7c81dde38ef7136baa3b1044f313',
-  COORDINATOR: '0x0f019Ade1D34399D946CF2f161386362655Dd1A4',
+  COORDINATOR: '0x8E49989F9D3aD89c8ab0de21FbA2E00C67ca872F',


Opened #310. The bulk of this module should be externalized to the NPM package. See also
nucypher/nucypher-contracts#124 for on-going discussion

KPrasch

It's so refreshing to see the demos so buttoned up and organized. 🤠

KPrasch · 2023-10-05T11:03:25Z

demos/README.md

+- [`nucypher-ts-demo`](https://github.com/nucypher/nucypher-ts-demo) - A demo of PRE in the `nucypher-ts` library.
+- [`tdec-sandbox`](https://github.com/nucypher/tdec-sandbox) - A demo of tDec in the `nucypher-ts` library.
+- [`tdec-nft-example`](https://github.com/nucypher/tdec-nft-example) - A demo of tDec in the `nucypher-ts` library.


In the spirit of the monorepo, shall we also internalize these demos to this repo?

Internalizing nucypher-ts-demo will require releasing @nucypher/pre, which I think we should postpone. tdec-sandbox and tdec-nft-example will be deprecated (archived) and replaced by existing demos.

I also generally agree with the division of examples/demos mentioned above 👍🏻

tdec-sandbox and tdec-nft-example will be deprecated (archived) and replaced by existing demos.

^ If we are going to deprecate/archive these, should we remove these in the readme then?

I will remove them from the README.md when we have a working alternative.

These things are easy to forget. If you are going to leave them for now, I'd recommend filing an issue and setting the appropriate project on it, so that it can be followed up on.

Tracked in #308

demos/taco-demo/README.md

derekpierre · 2023-10-05T13:21:08Z

demos/taco-demo/README.md

+### Polygon
+
+`@nucypher/taco` is in an early release. We recommend **not** using it in
+production _just yet_.


Do we need this for a demo readme?

derekpierre · 2023-10-05T13:24:21Z

demos/taco-demo/src/App.tsx

+    await switchNetwork(Mumbai.chainId);
+
+    const provider = new ethers.providers.Web3Provider(window.ethereum);
+    const ritualId = 2; // Replace with your own ritual ID


Is it possible to make this some sort of UI input, or browser var or some variable that users can easily set somewhere eg. some kind of .env file equivalent for local scripts? Even if localized to a specific file.

Whatever process for updating this number should end up in the readme/demo instructions.

derekpierre · 2023-10-05T13:28:16Z

demos/taco-nft-demo/README.md

+### Polygon
+
+`@nucypher/taco` is in an early release. We recommend **not** using it in
+production _just yet_.


Same question as in the other demo.

derekpierre · 2023-10-05T13:29:22Z

demos/taco-nft-demo/src/NFTConditionBuilder.tsx

+  enabled,
+}: Props) => {
+  const { library } = useEthers();
+  const goerliNFTAddress = '0x932Ca55B9Ef0b3094E8Fa82435b3b4c50d713043'; // https://goerli-nfts.vercel.app/


Tapir is going to use sepolia (lynx will still use goerli) - so we'll need a new NFT address on sepolia.

They both will still use Mumbai, so if using an NFT on mumbai is simpler then that works too.

(cc @KPrasch )

I guess we can keep this one around for use with Lynx, but yeah a new NFT contract on Sepolia will be needed, or as @derekpierre mentioned, just switch to a mumbai condition.

I will update the demos when we release TACo on Sepolia. I always have to test them in cases like this anyway.

derekpierre · 2023-10-05T13:35:22Z

examples/taco/nextjs/src/app/page.tsx

+    const messageKit = await encrypt(provider, message, hasPositiveBalance, ritualId, signer);
+
+    console.log('Decrypting message...');
+    const porterUri = getPorterUri('lynx'); // Test network
+    const decryptedMessage = await decrypt(provider, messageKit, signer, porterUri);


These API calls look great!

derekpierre · 2023-10-05T13:35:48Z

examples/taco/nextjs/src/app/page.tsx

+        value: 0,
+      },
+    });
+    const ritualId = 2; // Replace with your own ritual ID


Similar question about making this some sort of UI input/variable

packages/pre/src/policy.ts

derekpierre · 2023-10-05T13:53:14Z

packages/shared/abis/Coordinator.json

@@ -1,1375 +1,1423 @@
 [


Nice - this thought crossed my mind as well.

packages/shared/src/contracts/ethers-typechain/factories/Coordinator__factory.ts

derekpierre · 2023-10-05T14:00:07Z

packages/taco/src/conditions/base/rpc.ts

@@ -11,7 +11,9 @@ export const rpcConditionSchema = z.object({
  conditionType: z.literal(RpcConditionType).default(RpcConditionType),
  chain: createUnionSchema(SUPPORTED_CHAIN_IDS),
  method: z.enum(['eth_getBalance', 'balanceOf']),
-  parameters: z.array(EthAddressOrUserAddressSchema),
+  parameters: z.array(
+    z.union([EthAddressOrUserAddressSchema, z.string(), z.number()]),


Why not just EthAddressOrUserAddressSchema? Why also number or string?

PS. love how that typing reads.

The second parameter to eth_getBalance is blockNumer, which is a string (as per JSON RPC schema). I thought that accepting a number here would be more intuitive. Does that make sense? Or should we expose strict interfaces (lifted straight from JSON RPC)?

Oh right I got you.

But then we ideally want something like:

parameters = (EthAddressOrUserAddressSchema) OR (EthAddressOrUserAddressSchema, Union[string, int]])

So something like the following then (my zod is bad)?

z.array( z.union([ EthAddressOrUserAddressSchema, z.array( EthAddressOrUserAddressSchema, z.union([ z.string(), z.number()] ]) ) ]) )

If the above is the simplest way (perhaps not), might be cleaner to define a custom schema type somewhere else, similar to how we did EthAddressOrUserAddressSchema.

We should test this if we aren't already.

derekpierre · 2023-10-05T14:05:18Z

packages/taco/src/taco.ts

+    authSigner,
+  );
+
+  const isAuthorized = DkgCoordinatorAgent.isEncryptionAuthorized(


I think this check should be optional, and not done by default.

In some/most cases it is ok for Enrico to encrypt before being authorized. Authorization can be granted after encryption and the authorization check only really applies at decryption time (this was a prior question from BqEth).

I get the notion of being proactive about authorization before encryption, but they are different in their timing.

(cc @cygnusv , @KPrasch ).

Indeed, it's use-case specific, ritual-specific, and may actually be performed by different actors or user stories altogether.

Co-authored-by: David Núñez <david@nucypher.com>

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

cygnusv

I reviewed code and ran the demo locally. Worked like a charm. Awesome work, Piotr! We're almost there

derekpierre · 2023-10-06T12:30:06Z

packages/taco/src/conditions/base/rpc.ts

+  parameters: z.union([
+    z.array(EthAddressOrUserAddressSchema).length(1),
+    // Using tuple here because ordering matters
+    z.tuple([EthAddressOrUserAddressSchema, z.union([z.string(), z.number()])]),


Great - 🚀

derekpierre · 2023-10-06T12:30:20Z

packages/taco/test/conditions/base/rpc.test.ts

@@ -34,4 +35,52 @@ describe('validation', () => {
      },
    });
  });
+
+  describe('parameters', () => {


Love the testing!

piotr-roslaniec force-pushed the refactor-packages branch from 3d564b6 to f21b9f1 Compare September 25, 2023 14:26

This was referenced Sep 25, 2023

Migrate to a monorepo #297

Merged

Update v7.0 demos/examples #194

Closed

piotr-roslaniec force-pushed the refactor-packages branch 2 times, most recently from c75113e to b4e1570 Compare September 28, 2023 08:08

piotr-roslaniec marked this pull request as ready for review October 2, 2023 08:47

piotr-roslaniec requested review from jMyles, KPrasch, cygnusv, theref, derekpierre and manumonti October 2, 2023 08:48

piotr-roslaniec force-pushed the refactor-packages branch 2 times, most recently from a0f1c49 to d344181 Compare October 4, 2023 09:09

piotr-roslaniec mentioned this pull request Oct 4, 2023

Support encryptor allowlist #306

Closed

piotr-roslaniec force-pushed the refactor-packages branch from d344181 to d4a2d0c Compare October 5, 2023 08:34

piotr-roslaniec mentioned this pull request Oct 5, 2023

Migrate Node v16.x to v18.x #272

Closed

piotr-roslaniec force-pushed the refactor-packages branch from d4a2d0c to ada6fda Compare October 5, 2023 08:44

piotr-roslaniec mentioned this pull request Oct 5, 2023

Setup changesets for the initial release #309

Closed

cygnusv reviewed Oct 5, 2023

View reviewed changes

KPrasch approved these changes Oct 5, 2023

View reviewed changes

piotr-roslaniec mentioned this pull request Oct 5, 2023

Update Porter network configuration for @nucypher/* packages #311

Closed

theref approved these changes Oct 5, 2023

View reviewed changes

derekpierre reviewed Oct 5, 2023

View reviewed changes

demos/taco-demo/README.md Outdated Show resolved Hide resolved

derekpierre reviewed Oct 5, 2023

View reviewed changes

piotr-roslaniec mentioned this pull request Oct 5, 2023

Expose Porter domains using helper methods #312

Merged

1 task

derekpierre reviewed Oct 5, 2023

View reviewed changes

piotr-roslaniec and others added 18 commits October 5, 2023 22:11

chore(refactor): divide shared package into pre and taco

5927d78

chore(examples): add taco examples

a5e6d8b

chore(test): add multichain condition tests

da71bf0

chore(deps): deprecate support for node 16

7de1be7

chore: fix vitest test config

58254d0

feat(alpha-13): update coordinator contract

737b215

feat(alpha-13): check authorization before encrypting

71b3089

chore(examples): update taco/nodejs example

2aa2720

chore(demos): add demos/taco-demo

1292f31

chore(examples): update examples for alpha-13

5051b03

chore: remove redundant lockfile

a417a00

chore: document legacy demos

ed40754

chore(demos): add demos/taco-nft-demo

719f6a0

feat!: deprecate pre-tdec

f3fc336

Update examples/README.md

7752c24

Co-authored-by: David Núñez <david@nucypher.com>

Update examples/taco/webpack-5/src/index.html

9042753

Co-authored-by: David Núñez <david@nucypher.com>

Update demos/taco-demo/README.md

d5c70a5

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

fix(test): fix pre tests after a bad rebase

acb0fa1

piotr-roslaniec force-pushed the refactor-packages branch from 1f30886 to acb0fa1 Compare October 6, 2023 09:18

piotr-roslaniec added 3 commits October 6, 2023 11:27

feature(demos): add ritual id input to demos

fbc90e1

fix(conditions): rpc condition parameters not checking length

d2c4bf7

feature(allowlist): expose a method to check encrypter authorization

0a13562

cygnusv approved these changes Oct 6, 2023

View reviewed changes

piotr-roslaniec merged commit 60eacdb into nucypher:monorepo Oct 6, 2023
1 check passed

piotr-roslaniec deleted the refactor-packages branch October 6, 2023 12:12

piotr-roslaniec mentioned this pull request Oct 6, 2023

Consider providing read-to-use encrypters on testnet environments #313

Closed

derekpierre reviewed Oct 6, 2023

View reviewed changes

This was referenced Oct 18, 2023

Deprecate PRE-tDec #303

Closed

Add a multichain condition example #274

Closed

Refactor packages APIs #301

Refactor packages APIs #301

Conversation

piotr-roslaniec commented Sep 22, 2023 • edited Loading

codecov-commenter commented Sep 22, 2023 • edited Loading

Codecov Report

cygnusv 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

cygnusv Oct 5, 2023 • edited by KPrasch Loading

Choose a reason for hiding this comment

KPrasch Oct 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch 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

derekpierre Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

KPrasch Oct 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cygnusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec commented Sep 22, 2023 •

edited

Loading

codecov-commenter commented Sep 22, 2023 •

edited

Loading

cygnusv Oct 5, 2023 •

edited by KPrasch

Loading

KPrasch Oct 5, 2023 •

edited

Loading

derekpierre Oct 5, 2023 •

edited

Loading

derekpierre Oct 5, 2023 •

edited

Loading

KPrasch Oct 5, 2023 •

edited

Loading

derekpierre Oct 5, 2023 •

edited

Loading