Adds get_signing_cohort_conditions script #440
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new command-line script to retrieve signing cohort conditions from the SigningCoordinator contract. The script provides a CLI interface using Click to query cohort conditions by domain, cohort ID, and chain ID.
- Implements a new
get-cohort-conditionsCLI command using Click and Ape frameworks - Validates domain support and retrieves cohort conditions from the SigningCoordinator contract
- Outputs the retrieved conditions in formatted JSON
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if domain not in TESTNET_PROVIDERS: | ||
| raise click.ClickException(f"Unsupported domain: {domain}. Supported domains are: {', '.join(TESTNET_PROVIDERS)}") | ||
|
|
||
| print(f"Setting conditions for cohort {cohort_id} on {domain}:{network} with chain ID {chain_id}") |
There was a problem hiding this comment.
The message says 'Setting conditions' but this script only retrieves/gets conditions. The message should read 'Getting conditions for cohort...' to accurately reflect the script's purpose.
| print(f"Setting conditions for cohort {cohort_id} on {domain}:{network} with chain ID {chain_id}") | |
| print(f"Getting conditions for cohort {cohort_id} on {domain}:{network} with chain ID {chain_id}") |
| result = signing_coordinator.getSigningCohortConditions(cohort_id, chain_id) | ||
|
|
||
| print("Cohort Conditions:") | ||
| print(json.dumps(json.loads(result), indent=4)) |
There was a problem hiding this comment.
Calling json.loads() on result assumes it's a JSON string, but result is likely already a structured object from the contract call. This will fail if result is not a string. Either use json.dumps(result, indent=4, default=str) directly, or verify the actual return type of getSigningCohortConditions().
| print(json.dumps(json.loads(result), indent=4)) | |
| print(json.dumps(result, indent=4, default=str)) |
|
Needs a rebase. |
KPrasch
force-pushed
the
get_conditions
branch
from
9316139 to
0396b3d
Compare
| if domain not in TESTNET_PROVIDERS: | ||
| raise click.ClickException(f"Unsupported domain: {domain}. Supported domains are: {', '.join(TESTNET_PROVIDERS)}") |
There was a problem hiding this comment.
I don't think we need this check since domain is already restricted here - https://github.com/nucypher/nucypher-contracts/pull/440/files#diff-83ec17263e71b8869b9dfef8ee753eafc12b5d3b8b0a384e038dd0e29f809fb4R20.
| if domain not in TESTNET_PROVIDERS: | ||
| raise click.ClickException(f"Unsupported domain: {domain}. Supported domains are: {', '.join(TESTNET_PROVIDERS)}") | ||
|
|
||
| print(f"Setting conditions for cohort {cohort_id} on {domain}:{network} with chain ID {chain_id}") |
There was a problem hiding this comment.
| print(f"Setting conditions for cohort {cohort_id} on {domain}:{network} with chain ID {chain_id}") | |
| print(f"Getting conditions for cohort {cohort_id} on {domain}:{network} with chain ID {chain_id}") |
| signing_coordinator = registry.get_contract(domain=domain, contract_name="SigningCoordinator") | ||
|
|
||
| print("Getting conditions...") | ||
| print(f"Cohort ID: {cohort_id}, Chain ID: {chain_id}") |
There was a problem hiding this comment.
Seems redundant based on print statement on L47?
|
|
||
| with open(condition_file, 'r') as file: | ||
| condition = file.read().strip().encode("utf-8") | ||
| condition = json.dumps(json.loads(file.read().strip().encode("utf-8"))).encode("utf-8") |
There was a problem hiding this comment.
Is the first encode needed? i.e. should it be:
| condition = json.dumps(json.loads(file.read().strip().encode("utf-8"))).encode("utf-8") | |
| condition = json.dumps(json.loads(file.read().strip())).encode("utf-8") |
|
|
||
| if not condition: | ||
| raise click.ClickException("Condition file is empty or not provided.") | ||
|
|
There was a problem hiding this comment.
The check on L71&72 can be removed. Domain is already limited to specific values here - https://github.com/nucypher/nucypher-contracts/pull/440/files#diff-5b7786182e4b882eaf4ab7b7a4c02a0aca12a0c1df57054e720a370477fe5742R21
There was a problem hiding this comment.
L81 seems redundant given the print on L74.
2 participants
Type of PR:
Feature
Required reviews:
1