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

Remove obsolete script/transaction parameter check #1114

Merged
merged 1 commit into from Sep 13, 2021

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Aug 13, 2021

Closes #1099

Description

This check is now obsolete as we do check for importability of types at:

cadence/runtime/runtime.go

Lines 214 to 223 in fa0c3f6

if len(functionEntryPointType.Parameters) > 0 {
for _, param := range functionEntryPointType.Parameters {
if !param.TypeAnnotation.Type.IsImportable(map[*sema.Member]bool{}) {
err = &ScriptParameterTypeNotImportableError{
Type: param.TypeAnnotation.Type,
}
return nil, newError(err, context)
}
}
}

There are already tests available to cover the different cases:

t.Run("Resource", func(t *testing.T) {
t.Parallel()
script := `
pub fun main(arg: @Baz?) {
destroy arg
}
pub resource Baz {
}
`
err := executeScript(t, script, cadence.NewOptional(nil))
expectNonImportableError(t, err)
})


  • 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

@SupunS SupunS requested a review from turbolent as a code owner August 13, 2021 22:50
@SupunS SupunS self-assigned this Aug 13, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #1114 (1371033) into master (fa0c3f6) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1114   +/-   ##
=======================================
  Coverage   75.52%   75.53%           
=======================================
  Files         271      271           
  Lines       33322    33318    -4     
=======================================
- Hits        25167    25166    -1     
+ Misses       7021     7018    -3     
  Partials     1134     1134           
Flag Coverage Δ
unittests 75.53% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/interpreter/interpreter.go 91.32% <ø> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa0c3f6...1371033. Read the comment docs.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Would be nice to get this into 0.19

@SupunS SupunS merged commit 05fdc6c into master Sep 13, 2021
@SupunS SupunS deleted the supun/script-params branch September 13, 2021 17:10
bors bot added a commit to onflow/flow-go that referenced this pull request Sep 16, 2021
1283: Auto Cadence Update: Remove obsolete script/transaction parameter check r=turbolent a=turbolent

Auto generated PR to update Cadence version.

References: onflow/cadence#1114

Co-authored-by: turbolent <turbolent@users.noreply.github.com>
Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com>
Co-authored-by: Kay-Zee <Kay-Zee@users.noreply.github.com>
Co-authored-by: SupunS <SupunS@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow AnyStruct as script parameter type
3 participants