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

Allow importing type and capability values #1202

Merged
merged 16 commits into from
Nov 5, 2021
Merged

Allow importing type and capability values #1202

merged 16 commits into from
Nov 5, 2021

Conversation

dsainati1
Copy link
Contributor

@dsainati1 dsainati1 commented Oct 28, 2021

Closes #491, #712

Description

This does two primary things:

  1. Change the representation of types in cadence.TypeValue and cadence.Capability to be a cadence.Type instead of a string. This also requires implementation of encoding and decoding types to and from JSON.

  2. Implement ImportType to convert a cadence.Type into a interpreter.StaticType

  3. Update importing to allow for the importing of capability and type values


  • 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

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.

Nice work!

encoding/json/decode.go Outdated Show resolved Hide resolved
encoding/json/decode.go Outdated Show resolved Hide resolved
encoding/json/decode.go Outdated Show resolved Hide resolved
runtime/convertValues.go Outdated Show resolved Hide resolved
encoding/json/encode.go Outdated Show resolved Hide resolved
encoding/json/decode.go Outdated Show resolved Hide resolved
encoding/json/encode.go Outdated Show resolved Hide resolved
encoding/json/decode.go Outdated Show resolved Hide resolved
values.go Show resolved Hide resolved
values.go Show resolved Hide resolved
@turbolent turbolent mentioned this pull request Oct 29, 2021
6 tasks
@dsainati1 dsainati1 changed the base branch from master to bastian/cadence-types-cleanup October 29, 2021 16:40
@dsainati1 dsainati1 changed the base branch from bastian/cadence-types-cleanup to master October 29, 2021 16:41
@dsainati1 dsainati1 changed the title Allow importing type values Allow importing type and capability values Oct 29, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2021

Codecov Report

Merging #1202 (bec485a) into master (d138ccf) will increase coverage by 0.23%.
The diff coverage is 82.57%.

❗ Current head bec485a differs from pull request most recent head 314f5fa. Consider uploading reports for the commit 314f5fa to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1202      +/-   ##
==========================================
+ Coverage   77.18%   77.41%   +0.23%     
==========================================
  Files         273      273              
  Lines       34209    34726     +517     
==========================================
+ Hits        26403    26882     +479     
- Misses       6736     6768      +32     
- Partials     1070     1076       +6     
Flag Coverage Δ
unittests 77.41% <82.57%> (+0.23%) ⬆️

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

Impacted Files Coverage Δ
runtime/sema/type.go 87.92% <ø> (+0.12%) ⬆️
encoding/json/decode.go 73.18% <62.10%> (-7.64%) ⬇️
types.go 74.19% <71.42%> (+16.46%) ⬆️
runtime/interpreter/interpreter.go 89.03% <77.77%> (+0.11%) ⬆️
runtime/interpreter/statictype.go 69.86% <78.78%> (+0.96%) ⬆️
runtime/convertTypes.go 79.15% <97.61%> (+17.25%) ⬆️
encoding/json/encode.go 93.42% <98.40%> (+2.30%) ⬆️
runtime/convertValues.go 76.60% <100.00%> (+1.43%) ⬆️
runtime/interpreter/dynamictype.go 68.57% <100.00%> (+5.71%) ⬆️
runtime/interpreter/value.go 82.41% <100.00%> (+0.22%) ⬆️
... and 8 more

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 121981c...314f5fa. 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.

Nice work also adding support for importing capabilities

encoding/json/decode.go Show resolved Hide resolved
encoding/json/encode.go Outdated Show resolved Hide resolved
encoding/json/encode.go Outdated Show resolved Hide resolved
encoding/json/encode.go Outdated Show resolved Hide resolved
encoding/json/encode.go Outdated Show resolved Hide resolved
encoding/json/encode.go Show resolved Hide resolved
runtime/convertValues.go Outdated Show resolved Hide resolved
runtime/convertValues.go Outdated Show resolved Hide resolved
Copy link
Member

@SupunS SupunS 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!

Shall we also address this TODO and the one below that: convertValues_test.go#L1840-L1841
This was added earlier when we didn't have the type/capability importing support.

runtime/interpreter/dynamictype.go Show resolved Hide resolved
runtime/convertValues_test.go Outdated Show resolved Hide resolved
runtime/convertValues_test.go Outdated Show resolved Hide resolved
runtime/convertValues_test.go Outdated Show resolved Hide resolved
runtime/convertValues_test.go Show resolved Hide resolved
runtime/convertValues_test.go Show resolved Hide resolved
runtime/convertValues_test.go Outdated Show resolved Hide resolved
runtime/convertValues_test.go Outdated Show resolved Hide resolved
runtime/convertValues_test.go Show resolved Hide resolved
runtime/convertValues_test.go Outdated Show resolved Hide resolved
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! The PR is only missing some type conversions, which could also be done in a follow up PR, and some of Supun's suggestions are still open

runtime/convertTypes.go Show resolved Hide resolved
runtime/convertValues.go Outdated Show resolved Hide resolved
runtime/convertValues_test.go Outdated Show resolved Hide resolved
runtime/convertValues_test.go Outdated Show resolved Hide resolved
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.

@dsainati1 This looks like it is working now, but I still suggest applying Supun's code formatting suggestions. We try to keep lines short for readability reasons

runtime/convertValues_test.go Show resolved Hide resolved
runtime/interpreter/interpreter.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Language Breaking Change Breaks Cadence contracts deployed on Mainnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow import of type values (cadence.TypeValue)
4 participants