-
Notifications
You must be signed in to change notification settings - Fork 140
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
Validate arguments passed into cadence programs #724
Conversation
5c59bea
to
ee7ded2
Compare
ee7ded2
to
037b0b0
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.
This is great @SupunS, thank you for noticing this and implementing this all! 👏
@@ -378,7 +379,7 @@ func importValue(value cadence.Value) interpreter.Value { | |||
} | |||
case cadence.Enum: | |||
return importCompositeValue( | |||
common.CompositeKindStructure, |
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.
👍
059ff4c
to
36327c9
Compare
@turbolent Thanks for the review! As suggested, I moved the conformance checking to the
Can you please have a look at the new changes too? |
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.
Thank you for refactoring this @SupunS!
I thought about this a bit more and have the feeling this could be simplified a lot:
Currently adding this feature requires a lot of code to be added, but we already have most of the same checks in interpreter.IsSubType
, the function that validates if a dynamic type (which should have all necessary information about a value) to a static type.
OK If I'll try to see if I can maybe reduce the code a bit and rely on what we already have? I'd open another PR against this one
runtime/convertValues_test.go
Outdated
`pub fun main(arg: %[1]s)%[2]s { | ||
|
||
if !arg.isInstance(Type<%[1]s>()) { |
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.
Good idea to check both argument validation (value conforms to static type) and also check dynamic type conformance 👍
|
||
t.Parallel() | ||
|
||
var argumentPassingTests = []argumentPassingTest{ |
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.
👍
// Here it is assumed that imported values can only have static fields values, | ||
// but not computed field values. | ||
if v.Fields.Len() != len(compositeType.Fields) { |
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.
👍
runtime/interpreter/value.go
Outdated
compositeType, ok := dynamicType.(CompositeDynamicType) | ||
return ok && ValueConformsToSemaType(v, compositeType.StaticType) |
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 wonder if we should include composites' fields' dynamic types in the composite dynamic type, just like we include the dynamic types of arrays' elements, and then check them here?
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.
Yup, that would be ideal.
If we have that, we can also get rid of all the value.conformsToSemaType()
methods, because this is the only entry point to that method. For the numeric types, we could use isSubType()
runtime/interpreter/value.go
Outdated
} | ||
} | ||
|
||
func (v PathValue) conformsToSemaType(semaType sema.Type) bool { |
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.
👍
Sure, please do! I also had a look at the Also maybe its worth merging #732 prior to this, as it also removes a few (a lot, infact) changes added by this PR (pretty much all the numeric types get reduced to just two) UPDATE: It doesn't affect much of the changes, as the refactoring was mostly for the types, and has only a slight impact/conflict on the values. |
Right, I was mostly thinking the same, but then realized how much overlap / duplication there is and it might be good to re-consider.
Please do @SupunS! |
8d1a6d5
to
d44e8d1
Compare
use the same combination of dynamic type check and check that the value conforms to the dynamic type check it claims to be when checking if a composite value conforms to a composite dynamic type
c4b03d5
to
cdcb062
Compare
cdcb062
to
7799b19
Compare
Thanks, @turbolent for the cleanup! I merged your PR, and synched/rebased this branch with the master. Opened #777 to handle the potential cyclic references. |
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.
Looks good, great work! 👍
Closes #689
Description
This PR adds validation for the arguments passed into a cadence program, which checks the value against the dynamic type of the value.
For contributor use:
master
branchFiles changed
in the Github PR explorer