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

Use @nucypher/nucypher-contracts contract registry #314

Merged
merged 8 commits into from
Oct 17, 2023

Conversation

piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented Oct 6, 2023

Type of PR:

  • Feature

Required reviews:

  • 2

What this does:

  • Replaces contract addresses, ABIs, etc. stored in the repository with artifacts from @nucyper/nucypher-contracts
  • Exposes domain: Domain from user-facing API
  • Updates typechain usage for contract typings generation

Issues fixed/closed:

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2023

Codecov Report

Merging #314 (ed55e6b) into monorepo (9c172be) will decrease coverage by 0.91%.
The diff coverage is 53.94%.

@@             Coverage Diff              @@
##           monorepo     #314      +/-   ##
============================================
- Coverage     88.25%   87.34%   -0.91%     
============================================
  Files            33       33              
  Lines          2409     2465      +56     
  Branches        205      206       +1     
============================================
+ Hits           2126     2153      +27     
- Misses          254      282      +28     
- Partials         29       30       +1     
Files Coverage Δ
packages/pre/src/characters/alice.ts 87.74% <100.00%> (+0.07%) ⬆️
packages/pre/src/characters/bob.ts 85.61% <100.00%> (ø)
packages/pre/src/cohort.ts 89.28% <100.00%> (ø)
packages/taco/src/conditions/const.ts 100.00% <100.00%> (ø)
packages/taco/src/index.ts 100.00% <100.00%> (ø)
packages/taco/src/tdec.ts 93.71% <100.00%> (+0.16%) ⬆️
packages/pre/src/kits/retrieval.ts 77.27% <0.00%> (ø)
packages/pre/src/policy.ts 82.02% <60.00%> (-1.02%) ⬇️
packages/taco/src/dkg.ts 35.16% <8.33%> (-1.26%) ⬇️
packages/taco/src/taco.ts 83.82% <50.00%> (-12.30%) ⬇️

Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-35k lines 😆

Comment on lines +15 to +24
const parseContractRegistry = (registry: ContractRegistry): Contract[] =>
Object.keys(registry)
.map((chainId) => {
const networkRegistry = registry[chainId];
return Object.keys(networkRegistry).map((contractName): Contract => {
const contract = networkRegistry[contractName];
return { name: contractName as ContractName, abi: contract.abi };
});
})
.flat();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see this data model from nucypher-contracts adopted here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Happy to see these files go away.

import { DEFAULT_WAIT_N_CONFIRMATIONS, getContract } from '../registry';

export class GlobalAllowListAgent {
public static async registerEncrypters(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to be this PR, but you may want to add a follow-up issue to add the ability to check if an encrypter is already registered i.e. isAddressAuthorized(ritualid, address) I think the contract method is called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use it to check if the encrypter is already authorized before performing the authorization below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two methods are exposed by taco. I think the latter (registering encrypters) could use the former under the hood.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isEncryptionAuthorized on the Coordinator contract is different to isAddressAuthorized on the GlobalAllowList contract. The former is done during decryption because it needs the ciphertext digest etc.

The latter is a check for determining if a specific encryptor address is already authorized and doesn't need any ciphertext. See example here - https://github.com/nucypher/nucypher/blob/development/scripts/hooks/nucypher_dkg.py#L336.

) => DkgCoordinatorAgent.isEncryptionAuthorized(provider, ritualId, messageKit);
) => DkgCoordinatorAgent.isEncryptionAuthorized(provider, domain, ritualId, messageKit);

export const registerEncrypters = async (
Copy link
Member

@derekpierre derekpierre Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very specific to the GlobalAllowList. I think we need to be careful here. Rituals may use the GlobalAllowList but I don't believe they have to. We'd need to first check here whether the accessController for the associated ritual is the global allow list contract before registering the encrypter. It doesn't have to be, although more often than not, it may be.

cc @KPrasch , @cygnusv

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the encryptor onboarding is a bit more complicated and it depends on #306

We could hide this method from the taco API until we sort it out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I think that would be prudent.

@@ -6,7 +6,8 @@
<script src="index.js"></script>
</head>
<body>
<noscript>This page contains webassembly and javascript content, please enable
<noscript
>This page contains webassembly and javascript content, please enable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this <noscript > is done by a linter, but it looks pretty ugly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I could play around with linter rules, but I don't think these files are read as frequently.

"typedoc": "typedoc"
},
"dependencies": {
"@ethersproject/abi": "^5.7.0",
"@ethersproject/providers": "^5.7.2",
"@nucypher/nucypher-contracts": "^7.0.0-rc.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay!

@@ -40,5 +40,5 @@ export type GetContractTypeFromFactory<F> = F extends MinEthersFactory<
: never;

export type GetARGsTypeFromFactory<F> = F extends MinEthersFactory<any, any>
? Parameters<F["deploy"]>
? Parameters<F['deploy']>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: if these files are generated automatically, why the linting change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an oversight on my part. They should be generated and formatted in one step (to prevent line changes), but I haven't set it up yet.

Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎸 - good work.

Approved, but let's hide the registerEncrypters function from the API for now, before merging, until we sort out the proper logic. Let's ensure there's a follow-up issue for tracking.

@piotr-roslaniec piotr-roslaniec merged commit d52f9e6 into nucypher:monorepo Oct 17, 2023
2 checks passed
@piotr-roslaniec piotr-roslaniec deleted the contract-registry branch October 17, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

5 participants