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

Default Chain ID value in conditions #114

Closed
Tracked by #177
manumonti opened this issue Nov 23, 2022 · 5 comments · Fixed by #118
Closed
Tracked by #177

Default Chain ID value in conditions #114

manumonti opened this issue Nov 23, 2022 · 5 comments · Fixed by #118

Comments

@manumonti
Copy link
Member

manumonti commented Nov 23, 2022

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?

_Originally posted by @manumonti [and discussion] in #179

@manumonti manumonti mentioned this issue Nov 23, 2022
@piotr-roslaniec
Copy link
Contributor

piotr-roslaniec commented Nov 23, 2022

I would favor removing the default value, I think it only adds confusion to the Condition usage. In that case, we should probably add some validation with a list (enum) of supported networks.

@theref
Copy link
Contributor

theref commented Nov 23, 2022

I would be against adding some validation. That is done in nucypher/nucypher and the errors should bubble up. Adding more validation here means more things need to be kept in sync between the two repos

@piotr-roslaniec
Copy link
Contributor

Adding more validation here means more things need to be kept in sync between the two repos

We're already validating certain shared structures (such as Condition), and for a good reason - Since we're building a user-facing client I would prefer to fail early, especially since we don't enforce versioning on client-level #170

@cygnusv
Copy link
Member

cygnusv commented Nov 23, 2022

I would be against adding some validation. That is done in nucypher/nucypher and the errors should bubble up. Adding more validation here means more things need to be kept in sync between the two repos

But we should fail earlier, since conditions are associated to ciphertexts and it may be too late to change the condition later.

@KPrasch
Copy link
Member

KPrasch commented Nov 23, 2022

Agree. It's more efficient and better UX to prevent the submission of invalid conditions. However I think there is a little room for flexibility with regard to chain ID since we may not have a homogeneous deployment where all nodes are serving all supported chains.

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 a pull request may close this issue.

5 participants