nucypher / taco-web

🌮 A TypeScript client for TACo (Threshold Access Control)
https://docs.threshold.network/app-development/threshold-access-control-tac
GNU General Public License v3.0
15 stars 23 forks source link

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

Closed piotr-roslaniec closed 1 year ago

piotr-roslaniec commented 1 year ago
theref commented 1 year ago

Ritual Initiation

We already have Cohorts and Strategies so this will stay unchanged!

import { Cohort, Strategy } from '@nucypher/nucypher-ts';

const config = {
  threshold: 3,
  shares: 5,
  porterUri: 'https://porter-tapir.nucypher.community',
};
const newCohort = await Cohort.create(config);
const newStrategy = Strategy.create(newCohort);

import detectEthereumProvider from '@metamask/detect-provider';
import { providers } from 'ethers';

const MMprovider = await detectEthereumProvider();
const mumbai = providers.getNetwork(80001);

const web3Provider = new providers.Web3Provider(MMprovider, mumbai);
const newDeployed = await newStrategy.deploy('test', web3Provider);

The background work will be very different though. newStrategy.deploy needs to call initiateRitual on the Coordinator contract.

This also means that newStrategy will be unusable until the ritual has completed successfully (this is similar to having to deploy the strategy previously, however the await will now take much longer. Therefore the Strategy class probably needs to have a getStatus function that returns one of:

enum RitualState {
    NON_INITIATED,
    AWAITING_TRANSCRIPTS,
    AWAITING_AGGREGATIONS,
    TIMEOUT,
    INVALID,
    FINALIZED
}

Strategy class will also contain the ritualID, this should probably be exposed.

theref commented 1 year ago

Another option for Ritual Initiation...

We don't handle it within the strategy because this could become restrictive. Instead we just give detailed examples of how to achieve the functionality with standard tools (ethersjs, hardhat, truffle, etc):

import { Cohort, CoordinatorABI, CoordinatorAddress } from '@nucypher/nucypher-ts';

const config = {
  threshold: 3,
  shares: 5,
  porterUri: 'https://porter-tapir.nucypher.community',
};
const newCohort = await Cohort.create(config);
const nodes = newCohort.ursulaAddresses;

import { ethers } from "ethers";
const provider = new ethers.providers.Web3Provider(window.ethereum);
const coordinatorContract = new ethers.Contract(CoordinatorAddress, CoordinatorABI, provider);

// currently only connected via provider which is read-only, we need to connect using the signer
const signer = provider.getSigner();
const CoordinatorWithSigner = coordinatorContract.connect(signer);

// initiate ritual
tx = await CoordinatorWithSigner.initiateRitual(nodes);

There are similar questions regarding PSS and how that status is handled.

theref commented 1 year ago

Another question... Cohort and Strategy are really just configuration objects, but which should handle the interface to the DKG ritual? In the comments above I outlined an example where it was contained within Strategy, but there's an argument that it should be part of the Cohort.

Thus const newCohort = await Cohort.create(config); would be calling initiateRitual on the Coordinator contract. A Strategy would no longer need to be deployed, so whilst it would be a breaking api change, the overall flow would be simpler. At this point a reasonable question become, what is the point of Strategy then? are there other configuration options that it should contain? (default conditions is a good example).

piotr-roslaniec commented 1 year ago

Currently, Cohort represents a set of nodes that may be re-used in multiple CBD strategies. The same remains true for rituals - we could have a bunch of hand-picked nodes from which we draw ritual participants. But in order to make a decision on that design we need to have a better understanding of how node sampling is supposed to work in DKG.

arjunhassard commented 1 year ago
piotr-roslaniec commented 1 year ago

Marking this issue as a blocker for https://github.com/nucypher/nucypher-ts/pull/197

Need further clarity on the design of CBD API in nucypher-ts:

piotr-roslaniec commented 1 year ago

This issue is updated by changes in https://github.com/nucypher/nucypher-ts/pull/210

Introduces a separate CbdStrategy and PreStrategy with their corresponding sub-types as an initial draft of the new API

KPrasch commented 1 year ago

This issue can now be understood as two separate efforts:

  1. Remove PRE-Adapted-tDec
  2. Design & Implement CBD API
piotr-roslaniec commented 1 year ago

Notes from https://github.com/nucypher/nucypher-ts/pull/227

derekpierre commented 1 year ago

Some of my notes from observations made while working with nucypher-ts:

piotr-roslaniec commented 1 year ago

Addressed some comments

There are some outstanding talking points related to this issue. Form discussion on Discord:

1) Renaming of the nucypher-ts repository 2) Naming of the NuCypher product suite 3) Deprecating of PRE API in nucypher-ts (TBD) 4) Refactoring PRE API into a separate package under nucypher-ts in the monorepo (TBD) 5) Continued support for PRE post-CBD (TBD) 6) Making high-level decisions and creating a timeline for 3), 4), 5) 7) Low-level naming decisions and low-level API design (Strategy vs Policy)

theref commented 1 year ago

Some more thought after chats with @arjunhassard @derekpierre @cygnusv @manumonti

How do we feel about removing some of the objects and taking a more functional approach? Those objects were required before because we carried around state, but that's no longer the case.

We're thinking something crazy simple along the lines of:

import taco

public_key = # read from Coordinator contract
conditions = # build some conditions
ciphertext = taco.encrypt("plaintext", public_key, conditions, porter_uri)
evidence = # generate signature, zk proof, whatever
plaintext = taco.decrypt(ciphertext, ritual_id, evidence, porter_uri)

I'm aware that this looks like python not ts, but the idea is the same

piotr-roslaniec commented 1 year ago

How do we feel about removing some of the objects and taking a more functional approach?

Sounds good to me. Are we still interested in making some of the protocol objects compatible with PRE?

Additional thoughts: Do we want to rely on configuration objects, like Cohort? Or do we hide them behind ritual initiation and refer to them using ritual_id? This question probably extends to at least a couple of other places.

# How do we smuggle cohort config here?
conditions = # Do we plug it into a condition?
ciphertext = taco.encrypt("plaintext", public_key, conditions, porter_uri, ...) # Or as a parameter here?
theref commented 1 year ago

Do you need to know the cohort here? or can you just encrypt with the public_key?

But yeah, there's definitely still some state around...

cygnusv commented 1 year ago

From my ETHBCN notes (sorry I didn't share this earlier, I thought I did 🙈 ):

Encrypt

Either the creator is blockchainy (which gives them access to the public key from the ritualID by querying the Coordinator):

    import { taco } from "@thresold-network/taco";
    messageKit = taco.encrypt(message, ritualId, web3Provider, conditions)

or not (i.e., the creator must know the public key):

    import { taco } from "@thresold-network/taco";
    messageKit = taco.encrypt(message, publicKey, conditions)

Decrypt

    import { taco } from "@thresold-network/taco";
    taco.decrypt(messageKit, porterURI, web3Provider)

 Notes

cygnusv commented 1 year ago

Potential iteration based on comments from discussion with @ghardin1314 @derekpierre @jMyles. Mayor changes in debate here:

Encrypt

3 possible approaches:

Decrypt

    import { taco } from "@thresold-network/taco";
    message = taco.decrypt(messageKit, porterURI, [signer])

Thoughts?

derekpierre commented 1 year ago

Thanks for posting this @cygnusv 🎸 !

3 possible approaches:

The approaches do not have to be exclusive, and providing more than one encrypt function is not a bad thing. 1) and 3) are pretty natural based on our API thus far.

Porter creator: Access to the public key from the ritualID by querying the Coordinator via Porter.

Just of note: at this current moment, Porter only connects to the Ethereum network and does not connect to the Polygon network at all, since it never had to previously. Of course, we control the code, and that can be changed if desired.

derekpierre commented 1 year ago

I guess, if we are providing 3) which is the lowest common function (and provides the most flexibility), then we would need to provide an API for getting the public key from a ritual Id...?

If we are, is that functional or Object-oriented, and does that remove the need for 1)? 1) would just be a convenience method at that point.

If functional, something like:

publicKey = taco.getPublicKey(ritualId, web3Provider)

Or, if object-oriented then something like:

coordinator = new Coordinator(web3Provider)
publicKey = coordinator.getPublicKey(ritualId)
...

There may be other methods on the object, like getRitual(ritualId), perhaps getThreshold(ritualId)...?

Thinking out loud here.

ghardin1314 commented 1 year ago

Just of note: at this current moment, Porter only connects to the Ethereum network and does not connect to the Polygon network at all, since it never had to previously. Of course, we control the code, and that can be changed if desired.

Not sure exactly the full logic of the signature validation on the Ursula side (do they make an RPC call to verify the block number/hash used?) but does the signature really need to be chain specific? Can all :userAddress verification signatures be against eth mainnet?

derekpierre commented 1 year ago

Not sure exactly the full logic of the signature validation on the Ursula side (do they make an RPC call to verify the block number/hash used?) but does the signature really need to be chain specific? Can all :userAddress verification signatures be against eth mainnet?

Here's the verification function on the Ursula side - https://github.com/nucypher/nucypher/blob/development/nucypher/policy/conditions/context.py#L30. It's basically an eth_account.recover_message(...) call.

For signing the chain id is included in the domain section as part of the EIP712 spec (as it pertains to replay attack). See Definition of domainSeparator section - https://eips.ethereum.org/EIPS/eip-712.

ghardin1314 commented 1 year ago

For signing the chain id is included in the domain section as part of the EIP712 spec (as it pertains to replay attack). See Definition of domainSeparator section - https://eips.ethereum.org/EIPS/eip-712.

True, but the chainId is mostly used when verifying the signature onchain to avoid cross chain replays. Since this is purely offchain signature verification, it does not really matter what the chainId is (or if there even is one)

derekpierre commented 1 year ago

True, but the chainId is mostly used when verifying the signature onchain to avoid cross chain replays. Since this is purely offchain signature verification, it does not really matter what the chainId is (or if there even is one)

Yep it doesn't really matter, but it does prevent reuse of the signature for other chains - forcing a new signing; unless that's the thing you are looking to avoid...

ghardin1314 commented 1 year ago

Yep it doesn't really matter, but it does prevent reuse of the signature for other chains - forcing a new signing; unless that's the thing you are looking to avoid...

Are you actually checking that anywhere though? Like if the Condition is an EvmCondition involving polygon, will Ursula throw an error if my signed message is against Eth mainnnet?

Also how does this work with cross/multi chain conditions (which is think are supported or planned to be?) Does Ursula need to verify a signature against each chain with a corresponding Condition?

derekpierre commented 1 year ago

Are you actually checking that anywhere though? Like if the Condition is an EvmCondition involving polygon, will Ursula throw an error if my signed message is against Eth mainnnet?

I haven't dug into the eth_account recover_message call but you're probably right that it may not be checked. I guess from an eth signing perspective, the chain doesn't matter with respect to wallet ownership - it's all the same.

ghardin1314 commented 1 year ago

I haven't dug into the eth_account recover_message call but you're probably right that it may not be checked. I guess from an eth signing perspective, the chain doesn't matter with respect to wallet ownership - it's all the same.

From the link you sent, it looks like the Ursula only checks to see if the signature itself is valid and has no validation about the other parameters passed in. In reality, this is no better than just using a random salt to sign and verify against.

Although I'm not sure the extra step of validating the block number/hash is really work the RPC call in this case. Either way, it doesnt seem to me that the signature validation really needs to match the chain in which the conditions refer to

piotr-roslaniec commented 1 year ago

3 possible approaches

We can expose a simplified API using existing primitives:

export interface TacoMessageKit {
  ciphertext: Ciphertext;
  aad: Uint8Array;
  decrypter: ThresholdDecrypter;
  conditions: ConditionExpression;
}

export const encrypt = async (
  message: string,
  conditions: ConditionExpression,
  ritualId: number,
  web3Provider: ethers.providers.Web3Provider
): Promise<TacoMessageKit> => {
  const cohort = await makeCohort([]);
  const strategy = CbdStrategy.create(cohort);
  const deployedStrategy = await strategy.deploy(web3Provider, ritualId);
  const { ciphertext, aad } = deployedStrategy
    .makeEncrypter(conditions)
    .encryptMessageCbd(message);

  return {
    ciphertext,
    aad,
    decrypter: deployedStrategy.decrypter,
    conditions,
  };
};

export const decrypt = async (
  messageKit: TacoMessageKit,
  web3Provider: ethers.providers.Web3Provider
): Promise<Uint8Array> => {
  return await messageKit.decrypter.retrieveAndDecrypt(
    web3Provider,
    messageKit.conditions,
    FerveoVariant.simple,
    messageKit.ciphertext,
    false,
  );
};

export const taco = {
  encrypt,
  decrypt,
};

Then, we can selectively expose them if needed:

import { Strategy } from "@thresold-network/core"; // common? shared?

Alternatively, we can eliminate those primitives either during refactoring to a more straightforward taco API or later.

ghardin1314 commented 1 year ago

That looks pretty good to me generally. Its been a week or so since I looked at this, do you need to deploy a new strategy every time you encrypt something? What exactly are you deploying here? And how would you encrypt using an existing strategy?

piotr-roslaniec commented 1 year ago

Strategy is "just a wrapper" on a ritual that produces an encrypter and a decrypter. When we run Strategy.deploy without the ritualId parameter, we start a new ritual. When we use a ritualId, we re-use an existing ritual.

do you need to deploy a new strategy every time you encrypt something?

We don't have to deploy a new strategy every time we encrypt something, we can re-use our ritual.

What exactly are you deploying here?

What we're deploying is a new DKG ritual. In case when we're re-using the existing ritual, we still want to run some checks in balances to validate its correctness. Perhaps the notion of "deploying" is not the best description. Or we could use another verb in case of re-using a ritual.

And how would you encrypt using an existing strategy?

To encrypt using an existing strategy, re-use ritualId.

The code example I shared is just for illustratory purposes, i.e. to show how we could implement that today without changes to the structure of the existing API. I want to iterate on it before we settle on the final design.

ghardin1314 commented 1 year ago

Ah gotcha, I do think deploy might be a bit misleading here as I associate it with writing something to the blockchain. Other than that I think it looks great

piotr-roslaniec commented 1 year ago

Closed by #263