Validate arguments passed into cadence programs #724

SupunS · 2021-03-23T05:15:20Z

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:

Targeted PR against master branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work
Code follows the standards mentioned here
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

turbolent

This is great @SupunS, thank you for noticing this and implementing this all! 👏

turbolent · 2021-04-01T00:20:15Z

runtime/convertValues.go

@@ -378,7 +379,7 @@ func importValue(value cadence.Value) interpreter.Value {
 		}
 	case cadence.Enum:
 		return importCompositeValue(
-			common.CompositeKindStructure,


runtime/runtime.go

runtime/argument_validation.go

SupunS · 2021-04-02T08:04:59Z

@turbolent Thanks for the review!

As suggested, I moved the conformance checking to the interpreter.Value by introducing the following two methods:

ConformsToDynamicType(dynamicType DynamicType) bool
conformsToSemaType(semaType sema.Type) bool - This is a package-private method because we should discourage using it directly, because:
- It doesn't check for subtyping. It is handled by a wrapper method (ValueConformsToSemaType) to avoid repetitive subtyping checks. Anyone who wishes to use this functionality should use ValueConformsToSemaType function instead.
- It is currently only implemented for the values that are importable. For the rest, e.g: FunctionValue, it doesn't do a deep value equality (we can gradually add these, but doesn't require for now).

Can you please have a look at the new changes too?

turbolent

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

turbolent · 2021-04-05T18:04:58Z

runtime/convertValues_test.go

+				`pub fun main(arg: %[1]s)%[2]s {
+
+					if !arg.isInstance(Type<%[1]s>()) {


Good idea to check both argument validation (value conforms to static type) and also check dynamic type conformance 👍

turbolent · 2021-04-05T18:05:13Z

runtime/convertValues_test.go

+
+	t.Parallel()
+
+	var argumentPassingTests = []argumentPassingTest{


turbolent · 2021-04-05T18:11:40Z

runtime/interpreter/value.go

+	// 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) {


runtime/interpreter/value.go

turbolent · 2021-04-05T18:20:44Z

runtime/interpreter/value.go

+	compositeType, ok := dynamicType.(CompositeDynamicType)
+	return ok && ValueConformsToSemaType(v, compositeType.StaticType)


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?

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

turbolent · 2021-04-05T18:28:02Z

runtime/interpreter/value.go

+	}
+}
+
+func (v PathValue) conformsToSemaType(semaType sema.Type) bool {


SupunS · 2021-04-05T19:11:21Z

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

Sure, please do!

I also had a look at the isSubType() method - why I couldn't use it because it mostly does a nominal type check, whereas here we actually do sort of a structural type check. But maybe worth looking from a fresh pair of eyes!

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.

turbolent · 2021-04-05T20:13:29Z

I also had a look at the isSubType() method - why I couldn't use it because it mostly does a nominal type check, whereas here we actually do sort of a structural type check. But maybe worth looking from a fresh pair of eyes!

Right, I was mostly thinking the same, but then realized how much overlap / duplication there is and it might be good to re-consider.

Also maybe its worth merging #732 prior to this,

Please do @SupunS!

turbolent · 2021-04-07T01:26:23Z

@SupunS Opened #776 against this PR to reduce the code, let me know what you think!

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

SupunS · 2021-04-07T05:46:38Z

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.

runtime/convertValues.go

turbolent

Looks good, great work! 👍

SupunS added the Improvement label Mar 23, 2021

SupunS requested a review from turbolent March 23, 2021 05:15

SupunS self-assigned this Mar 23, 2021

SupunS force-pushed the supun/improve-import-export-3 branch 2 times, most recently from 5c59bea to ee7ded2 Compare March 23, 2021 05:27

SupunS mentioned this pull request Mar 23, 2021

Fix runtime representation of native HashAlgorithm/SignAlgorithm enums #725

Merged

6 tasks

SupunS force-pushed the supun/improve-import-export-3 branch from ee7ded2 to 037b0b0 Compare March 23, 2021 06:13

SupunS added the Feature label Mar 29, 2021

turbolent reviewed Apr 1, 2021

View reviewed changes

SupunS marked this pull request as draft April 1, 2021 18:22

SupunS force-pushed the supun/improve-import-export-3 branch 3 times, most recently from 059ff4c to 36327c9 Compare April 2, 2021 07:44

SupunS marked this pull request as ready for review April 2, 2021 07:46

turbolent reviewed Apr 5, 2021

View reviewed changes

SupunS force-pushed the supun/improve-import-export-3 branch 2 times, most recently from 8d1a6d5 to d44e8d1 Compare April 6, 2021 03:20

turbolent mentioned this pull request Apr 7, 2021

Improve value type conformance check #776

Merged

6 tasks

SupunS added 8 commits April 7, 2021 10:56

Validate value conformance to the type

05472d4

Use dynamic type instead of static type for validation

e4708fa

Add argument validation tests

22d9b68

Add licence header

4ed2842

Revert type name refactoring

f60ceda

Update argument validation error

8c23269

Fix crypto enum raw value

d0bf1cd

Refactor code

bd4155a

SupunS and others added 5 commits April 7, 2021 10:56

Move conformance checks to the value structs

60dc212

Remove unused type name constants

95b503b

Rafactor code. Rename to array 'elements' from 'members'

1449340

ensure we don't forget to update the error message (again)

a54063b

remove ValueConformsToSemaType and conformsToSemaType

3b26bad

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

SupunS force-pushed the supun/improve-import-export-3 branch from c4b03d5 to cdcb062 Compare April 7, 2021 05:26

Use 'equal()' method to check numeric type equality

7799b19

SupunS force-pushed the supun/improve-import-export-3 branch from cdcb062 to 7799b19 Compare April 7, 2021 05:28

SupunS mentioned this pull request Apr 7, 2021

Handle cycles in dynamic type checking for ephemeral references #777

Closed

5 tasks

turbolent reviewed Apr 7, 2021

View reviewed changes

runtime/convertValues.go Outdated Show resolved Hide resolved

fix typo

78c6188

turbolent approved these changes Apr 7, 2021

View reviewed changes

SupunS merged commit 2881963 into master Apr 8, 2021

SupunS deleted the supun/improve-import-export-3 branch April 8, 2021 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate arguments passed into cadence programs #724

Validate arguments passed into cadence programs #724

SupunS commented Mar 23, 2021 •

edited

Loading

turbolent left a comment

turbolent Apr 1, 2021

SupunS commented Apr 2, 2021 •

edited

Loading

turbolent left a comment

turbolent Apr 5, 2021

turbolent Apr 5, 2021

turbolent Apr 5, 2021

turbolent Apr 5, 2021

SupunS Apr 6, 2021 •

edited

Loading

turbolent Apr 5, 2021

SupunS commented Apr 5, 2021 •

edited

Loading

turbolent commented Apr 5, 2021

turbolent commented Apr 7, 2021

SupunS commented Apr 7, 2021

turbolent left a comment

		`pub fun main(arg: %[1]s)%[2]s {

		if !arg.isInstance(Type<%[1]s>()) {


		t.Parallel()

		var argumentPassingTests = []argumentPassingTest{

		compositeType, ok := dynamicType.(CompositeDynamicType)
		return ok && ValueConformsToSemaType(v, compositeType.StaticType)

Validate arguments passed into cadence programs #724

Validate arguments passed into cadence programs #724

Conversation

SupunS commented Mar 23, 2021 • edited Loading

Description

turbolent left a comment

Choose a reason for hiding this comment

turbolent Apr 1, 2021

Choose a reason for hiding this comment

SupunS commented Apr 2, 2021 • edited Loading

turbolent left a comment

Choose a reason for hiding this comment

turbolent Apr 5, 2021

Choose a reason for hiding this comment

turbolent Apr 5, 2021

Choose a reason for hiding this comment

turbolent Apr 5, 2021

Choose a reason for hiding this comment

turbolent Apr 5, 2021

Choose a reason for hiding this comment

SupunS Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

turbolent Apr 5, 2021

Choose a reason for hiding this comment

SupunS commented Apr 5, 2021 • edited Loading

turbolent commented Apr 5, 2021

turbolent commented Apr 7, 2021

SupunS commented Apr 7, 2021

turbolent left a comment

Choose a reason for hiding this comment

SupunS commented Mar 23, 2021 •

edited

Loading

SupunS commented Apr 2, 2021 •

edited

Loading

SupunS Apr 6, 2021 •

edited

Loading

SupunS commented Apr 5, 2021 •

edited

Loading