Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Demo QA #179

Closed
arjunhassard opened this issue Nov 14, 2022 · 17 comments
Closed

Demo QA #179

arjunhassard opened this issue Nov 14, 2022 · 17 comments
Labels
documentation Improvements or additions to documentation

Comments

@arjunhassard
Copy link
Member

arjunhassard commented Nov 14, 2022

  • Look at documentation for step-by-step guide and suggest anything that's missing
  • Remove payment step
@piotr-roslaniec
Copy link
Contributor

piotr-roslaniec commented Nov 14, 2022

Old demo runbook. Could be useful for QA.

Maybe we could use something like cypress to do automated QA / demo testing. Not sure how to handle the web3 provider (MetaMask) thought.

@arjunhassard arjunhassard added the documentation Improvements or additions to documentation label Nov 14, 2022
@manumonti
Copy link
Member

manumonti commented Nov 22, 2022

While I was trying to build a new app, I found a lack of information in Get Started with TAC. This caused that I got stuck in the Step 2: Build a Cohort

IMHO, the use of WASM bindings should be documented here as a kind of "prerequisite" or "step 0". The problem I found trying to build a basic app is:

  1. nucypher-ts uses nucypher-core as a dependency. nucypher-core contains WASM bindings.
  2. To run nucypher-ts (and hence, WASM from nucypher-core) we need a bundler that understands what WASM is.
  3. Depending on what project we're running, two good options could be to use Webpack or Craco, but in any case, we need to tell them how to load WASM.

In any case, we have some examples of how to start building a new dApp here, here, and here, that could be linked in this documentation.

@piotr-roslaniec
Copy link
Contributor

IMHO, the use of WASM bindings should be documented here as a kind of "prerequisite" or "step 0". The problem I found trying to build a basic app is:

Maybe there is a better way (bundler-agnostic way) to incorporate nucypher-core into nucypher-ts.

@manumonti
Copy link
Member

manumonti commented Nov 22, 2022

Maybe there is a better way (bundler-agnostic way) to incorporate nucypher-core into nucypher-ts.

It sounds great since it could make it easier and faster for adopters to develop an app using nucypher-ts

@manumonti
Copy link
Member

I found a little bit confusing the default values taken in the Conditions doc:

There is no reference to what network will be taken by default. If you inspect the code, the Göerli testnet will be taken. So I'm wondering myself:

Should we have a default value for this? Or should be mandatory to specify the chain ID when creating the Conditions object? What should be the default value in the former case, Ethereum mainnet or Göerli?

@derekpierre
Copy link
Member

derekpierre commented Nov 22, 2022

Should we have a default value for this?

Great questions/thoughts. Given the eventual goal to support many different chains (polygon, mainnet etc.), having a "default" value doesn't make that much sense to me. My personal preference would be to not have a default value, and force the user to specify the chain ID. Apps like a Masterfile etc. could set a "default" chain id at their own layer.

@KPrasch
Copy link
Member

KPrasch commented Nov 22, 2022

Should we have a default value for this?

Great questions/thoughts. Given the eventual goal to support many different chains (polygon, mainnet etc.), having a "default" value doesn't make that much sense...

I completely agree. Additionally we need to document the supported chains.

@manumonti
Copy link
Member

Great questions/thoughts. Given the eventual goal to support many different chains (polygon, mainnet etc.), having a "default" value doesn't make that much sense to me. My personal preference would be to not have a default value, and force the user to specify the chain ID. Apps like a Masterfile etc. could set a "default" chain id at their own layer.

I completely agree. Additionally we need to document the supported chains.

I have created an issue in nucypher-ts: #114

@manumonti
Copy link
Member

I added these two issues to nucypher-ts repo:

Update main README file
Update references to IBEX network in code

@manumonti
Copy link
Member

I get an error when I try to deploy a new strategy using a network other than Rinkeby.

The documentation still includes Rinkeby as the network. When I try to deploy a new strategy following the docs 5. Build a Strategy, and I try to use Goerli, I get the error

Error
No contracts found for chainId: 5

I'm not sure what network to use here instead of Rinkeby.

@piotr-roslaniec
Copy link
Contributor

piotr-roslaniec commented Nov 25, 2022

@manumonti I think we're expecting the payment stub contract to be on Mumbai. The chain id specified by policy should refer to an on-chain state of Goerli.

@manumonti
Copy link
Member

manumonti commented Nov 28, 2022

Summary of issues found up to now:

@manumonti
Copy link
Member

manumonti commented Nov 28, 2022

I think we should at least mention in Get started that there are two possible networks. One for CBD/on-chain state (mainnet Ethereum, testnet Görli) and other for "payments" (mainnet Polygon, testnet Polygon Mumbai). It could be confusing to use Mumbai in Build a Strategy without any previous reference about this. Issue created here: #122

@manumonti
Copy link
Member

manumonti commented Nov 30, 2022

There is room to improve the code in section "4. Build a Strategy" (https://docs.threshold.network/app-development/threshold-access-control-tac/get-started-with-tac#4.-build-a-strategy).

Firstly, rinkeby references should be fixed as mentioned here: #122

Also, the import of "ethers" package is wrong. It should be something like

import { providers } from 'ethers';
[...]
const web3Provider = new providers.Web3Provider([...]);

Also, a note about Metamask network should be added: Polygon Mumbai network has to be previously selected on Metamask client or this code will fail. Another option is to use here @usedapp/core package as we use in this example: https://github.com/nucypher/tdec-nft-example/blob/2e8df0a55910a1ed34761aefa27c17f2633ff2fc/src/StrategyBuilder.tsx#L2

Issue created here: https://github.com/nucypher/tdec/issues/102

@manumonti
Copy link
Member

I have faced some problems related to RPC and timelock conditions. Described in this issue: #142

@derekpierre
Copy link
Member

Initial round of QA completed, with subsequent issues filed. For any subsequent rounds, we can open a new issue.

@arjunhassard arjunhassard transferred this issue from another repository Feb 16, 2023
@arjunhassard arjunhassard transferred this issue from another repository Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Completed
Development

No branches or pull requests

10 participants