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

Change Array.Get() to return Value instead of Storable #316

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

fxamacker
Copy link
Member

Closes #315
Updates #296 #292

Description

This PR changes Array.Get() to return Value instead of Storable.

Currently, Array.Get() returns Storable and client converts returned Storable to Value. However, it only makes sense to return Storable if client needs to remove register (slab) by StorageID (StorageIDStorable).

Get() should only provide Value without possibility of client manipulating the underlying Storable.

This is prep work for Atree Register Inlining (#292) and will also harden the API.


  • Targeted PR against main 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

@fxamacker fxamacker added improvement breaking change changes to API that can break programs using Atree's API labels Jun 27, 2023
@fxamacker fxamacker self-assigned this Jun 27, 2023
This commit changes Array.Get() to return Value instead of Storable.

Currently, Array.Get() returns Storable and client converts
returned Storable to Value.  However, it only makes sense to
return Storable if client needs to remove register (slab) by
StorageID (StorageIDStorable).

Get() should only provide value without possibility of client
manipulating the underlying storable.
@fxamacker fxamacker force-pushed the fxamacker/change-array-get-to-return-value branch from c182da1 to 044db01 Compare June 27, 2023 16:15
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

Merging #316 (044db01) into main (7c97278) will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
- Coverage   64.62%   64.60%   -0.03%     
==========================================
  Files          14       14              
  Lines        7991     8000       +9     
==========================================
+ Hits         5164     5168       +4     
- Misses       2152     2155       +3     
- Partials      675      677       +2     
Impacted Files Coverage Δ
array.go 69.80% <66.66%> (-0.07%) ⬇️

... and 2 files with indirect coverage changes

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!

@fxamacker fxamacker merged commit 0c73dea into main Jun 29, 2023
fxamacker added a commit to onflow/cadence that referenced this pull request Jun 29, 2023
This commit updates Cadence to use new Atree API
- Array.Get()
- OrderedMap.Get()
- MaxInlineArrayElementSize()
- MaxInlineMapKeySize()

For more info, see Atree PRs:
- onflow/atree#314
- onflow/atree#316
- onflow/atree#318
turbolent pushed a commit to onflow/cadence that referenced this pull request Jan 26, 2024
This commit updates Cadence to use new Atree API
- Array.Get()
- OrderedMap.Get()
- MaxInlineArrayElementSize()
- MaxInlineMapKeySize()

For more info, see Atree PRs:
- onflow/atree#314
- onflow/atree#316
- onflow/atree#318
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change changes to API that can break programs using Atree's API improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array.Get() should return Value instead of Storable
3 participants