-
Notifications
You must be signed in to change notification settings - Fork 22
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
Update Conditions API #267
Conversation
758e94d
to
d9718ed
Compare
d9718ed
to
8a8a67e
Compare
@@ -211,50 +212,6 @@ describe('condition set', () => { | |||
} | |||
); | |||
|
|||
it.each([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this test because we no longer duck-type conditions and rely on conditionType
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can repurpose to test to check what happens when the condition_type
is either not defined, or defined but the JSON does not match what's expected based on the type.
const ownsBufficornNFT = ERC721Ownership.fromObj({ | ||
const ownsBufficornNFT = new ERC721Ownership({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Condition constructors are now taking objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great 🙌
export const bytesEquals = (first: Uint8Array, second: Uint8Array): boolean => | ||
first.length === second.length && | ||
first.every((value, index) => value === second[index]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓
@@ -24,7 +24,6 @@ export { PolicyMessageKit } from './kits/message'; | |||
// Conditions | |||
import type { CustomContextParam } from './conditions'; | |||
import * as conditions from './conditions'; | |||
// TODO: Not sure how to re-export this type from the conditions module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-issue
// TODO: Consider replacing with `z.unknown`: | ||
// Since Solidity types are tied to Solidity version, we may not be able to accurately represent them in Zod. | ||
// Alternatively, find a TS Solidity type lib. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think you could probably replace pretty much anything having to do with interfacing with contracts with this maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's likely. I've already started looking into viem
: #271
.object({ | ||
name: z.string(), | ||
type: z.enum(EthBaseTypes), | ||
internalType: z.enum(EthBaseTypes), // TODO: Do we need to validate this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this contract validation seems a little bit tricky, and I think there is a low probability of having a bad ABI contract. I would leave this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it seems likely that ethers
or viem
could produce a useful error for the user if the abi isn't valid.
src/conditions/condition-expr.ts
Outdated
private static conditionFromObject(obj: ConditionProps): Condition { | ||
switch (obj.conditionType) { | ||
case 'rpc': | ||
return new RpcCondition(obj as RpcConditionProps); | ||
case 'time': | ||
return new TimeCondition(obj as TimeConditionProps); | ||
case 'contract': | ||
return new ContractCondition(obj as ContractConditionProps); | ||
case 'compound': | ||
return new CompoundCondition(obj as CompoundConditionProps); | ||
default: | ||
throw new Error(`Invalid conditionType: ${obj.conditionType}`); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RFC: Do we really need access to concrete types now that we have the conditionType
in every Condition
? For example, what is there to gain for our users from having an RpcCondition
instead of a Condition
instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good point. I like the simplicity of just having a condition
instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also +1 for having this simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to reiterate: "I, as a TS developer, don't need to know the concrete type of my condition
because I already know it's conditionType
". Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to reiterate: "I, as a TS developer, don't need to know the concrete type of my condition because I already know it's conditionType". Does that make sense?
Not sure I follow. What would be the outcome of such a plan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me rephrase that:
- I just deserialized a condition. I want to know what type of condition it is. I can just check (by hand or programmatically) what is the value of
conditionType
. That's all good, the problem is solved. - But now, why would I need to know the concrete TypeScript type of my conditions? And if I ever need to know that, I follow steps in 1) and cast my condition to the desired type,
condition as RpcCondition
(or any other condition, based onconditionType
). So, essentially what the code above is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to Condition.fromObj
8a8a67e
to
e5b0976
Compare
Hi @ghardin1314, you may be interested in reviewing this PR. |
Wishing I used valibot instead. Perhaps in a different life. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great job 👏. In really like this approach.
const ownsBufficornNFT = ERC721Ownership.fromObj({ | ||
const ownsBufficornNFT = new ERC721Ownership({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great 🙌
export const bytesEquals = (first: Uint8Array, second: Uint8Array): boolean => | ||
first.length === second.length && | ||
first.every((value, index) => value === second[index]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓
.object({ | ||
name: z.string(), | ||
type: z.enum(EthBaseTypes), | ||
internalType: z.enum(EthBaseTypes), // TODO: Do we need to validate this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this contract validation seems a little bit tricky, and I think there is a low probability of having a bad ABI contract. I would leave this as is.
src/conditions/condition-expr.ts
Outdated
private static conditionFromObject(obj: ConditionProps): Condition { | ||
switch (obj.conditionType) { | ||
case 'rpc': | ||
return new RpcCondition(obj as RpcConditionProps); | ||
case 'time': | ||
return new TimeCondition(obj as TimeConditionProps); | ||
case 'contract': | ||
return new ContractCondition(obj as ContractConditionProps); | ||
case 'compound': | ||
return new CompoundCondition(obj as CompoundConditionProps); | ||
default: | ||
throw new Error(`Invalid conditionType: ${obj.conditionType}`); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good point. I like the simplicity of just having a condition
instance.
src/conditions/condition-expr.ts
Outdated
private static conditionFromObject(obj: ConditionProps): Condition { | ||
switch (obj.conditionType) { | ||
case 'rpc': | ||
return new RpcCondition(obj as RpcConditionProps); | ||
case 'time': | ||
return new TimeCondition(obj as TimeConditionProps); | ||
case 'contract': | ||
return new ContractCondition(obj as ContractConditionProps); | ||
case 'compound': | ||
return new CompoundCondition(obj as CompoundConditionProps); | ||
default: | ||
throw new Error(`Invalid conditionType: ${obj.conditionType}`); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to reiterate: "I, as a TS developer, don't need to know the concrete type of my condition because I already know it's conditionType". Does that make sense?
Not sure I follow. What would be the outcome of such a plan?
@@ -211,50 +212,6 @@ describe('condition set', () => { | |||
} | |||
); | |||
|
|||
it.each([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you can repurpose to test to check what happens when the condition_type
is either not defined, or defined but the JSON does not match what's expected based on the type.
Codecov Report
@@ Coverage Diff @@
## alpha #267 +/- ##
==========================================
+ Coverage 80.63% 80.99% +0.36%
==========================================
Files 36 36
Lines 1012 1021 +9
Branches 112 110 -2
==========================================
+ Hits 816 827 +11
+ Misses 188 186 -2
Partials 8 8
|
fcc83eb
to
19d2a9e
Compare
src/types.ts
Outdated
@@ -8,3 +8,10 @@ export enum ChainId { | |||
GOERLI = 5, | |||
MAINNET = 1, | |||
} | |||
|
|||
export const SUPPORTED_CHAIN_IDS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is SUPORTED_CHAIN_IDS
used outstide of conditions? If so, it could be good to put it back in const
.
Same question for ChainId
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved SUPORTED_CHAIN_IDS
to const.ts
and ChainId
to web3.ts
src/zod.ts
Outdated
@@ -0,0 +1,31 @@ | |||
import { Primitive, z, ZodLiteral } from 'zod'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Should we move this to the conditions
directory? Unless Zod is used/will be used somewhere else?
}); | ||
}).toThrow(`Invalid conditionType: ${invalidConditionType}`); | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming not already tested somewhere else, perhaps you can also test providing a legit condition type, but the values provided don't match that type - is that possible?
For example, specify a time condition type, but provide valid contract condition values.
src/conditions/base/index.ts
Outdated
} | ||
} | ||
|
||
export { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the individual base schamas and props need to be exported, or is the specific condition classes (above) sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think props yes (because they make typing a bit easier), schemas - not so much (no direct value-add for users).
public readonly schema: ConditionSchema, | ||
public readonly value: ConditionProps | ||
) { | ||
const { data, error } = Condition.validate(schema, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/conditions/condition.ts
Outdated
public static validate( | ||
schema: ConditionSchema, | ||
value: ConditionProps, | ||
override: Partial<ConditionProps> = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the override? I assumed (perhaps mistakenly so) that this would match the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I don't think we need those anymore
f0b0b56
to
8ec9151
Compare
Bundled size for the package is listed below: build/main/src/characters: 74.22 KB |
8ec9151
to
b0b478f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @piotr-roslaniec
Thank you @ghardin1314 for the DX improvements
expect(result.error).toBeUndefined(); | ||
expect(result.data).toEqual({ | ||
...conditionObj, | ||
conditionType: 'compound', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the documentation and Conditions Schema, should we consider compound
to be a fourth condition type, or on a different 'level' from contract
, rpc
and time
? And does the constructor tell the developer that they're creating a compound condition set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think CompoundCondition
is somewhat different since (by definition) it can't be used on its own. That's why I didn't put it into conditions/base
which is supposed to contain some basic building blocks for conditions.
Yes, the constructor expects CompoundConditionProps
and provides type hints for TS developers.
b0b478f
to
a4c2b44
Compare
a4c2b44
to
84c8ef4
Compare
Type of PR:
Required reviews:
What this does:
conditionType
discriminating field to conditions*Props
Issues fixed/closed:
Why it's needed:
Notes for reviewers:
condition_type
to condition schemas nucypher#3201Condition
is now at.conditions.Condition
instead of.conditions.base.Condition
conditions/base/*.test.ts