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

Unexport SlabID fields to prevent misuse #323

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented Jul 6, 2023

Updates #296 #292 onflow/flow-go#1744
Updates onflow/cadence#2622 and more context at onflow/cadence#2622 (comment).

Description

Previously SlabID had two exported fields Address and Index. Address field was used in Cadence repo to obtain storage address.

We want clients to obtain storage address by calling Array.Address() or OrderedMap.Address(). This PR unexports Address field to prevent misuse.


  • 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 Jul 6, 2023
@fxamacker fxamacker self-assigned this Jul 6, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #323 (33b8013) into fxamacker/rename-id-and-related-functions-and-types (adcd6fa) will decrease coverage by 0.10%.
The diff coverage is 80.95%.

@@                                   Coverage Diff                                   @@
##           fxamacker/rename-id-and-related-functions-and-types     #323      +/-   ##
=======================================================================================
- Coverage                                                64.74%   64.64%   -0.10%     
=======================================================================================
  Files                                                       14       14              
  Lines                                                     8046     8050       +4     
=======================================================================================
- Hits                                                      5209     5204       -5     
- Misses                                                    2160     2168       +8     
- Partials                                                   677      678       +1     
Impacted Files Coverage Δ
array.go 69.92% <71.42%> (ø)
map.go 66.78% <80.00%> (-0.10%) ⬇️
storage.go 74.17% <81.81%> (-0.79%) ⬇️
basicarray.go 49.78% <100.00%> (ø)
storable.go 54.41% <100.00%> (ø)

fxamacker added 2 commits July 6, 2023 15:57
Currently, in order to create StorableSlab, user needs to:
- generate SlabID from storage
- create new StorableSlab
- store created StorableSlab in storage

This commit unexports StorableSlab fields and adds NewStorableSlab().
With this change, user can simply call NewStorableSlab().
ValueID identifies atree Array and OrderedMap while SlabID identifies
slab in storage.  SlabID should only be used to retrieve, store, and
remove slab in storage.
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.

👌

@fxamacker fxamacker merged commit 251b774 into fxamacker/rename-id-and-related-functions-and-types Jul 6, 2023
fxamacker added a commit to onflow/cadence that referenced this pull request Jul 7, 2023
This commit updates Cadence to use new Atree API
- Array.SlabID()
- OrderedMap.SlabID()
- SlabID
- SlabIndex
- etc.

For more info, see Atree PRs:
- onflow/atree#322
- onflow/atree#323
- onflow/atree#324
fxamacker added a commit to onflow/cadence that referenced this pull request Jul 7, 2023
This commit updates Cadence to use new Atree API
- Array.SlabID()
- OrderedMap.SlabID()
- SlabID
- SlabIndex
- etc.

For more info, see Atree PRs:
- onflow/atree#322
- onflow/atree#323
- onflow/atree#324
@fxamacker fxamacker mentioned this pull request Jul 7, 2023
6 tasks
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.SlabID()
- OrderedMap.SlabID()
- SlabID
- SlabIndex
- etc.

For more info, see Atree PRs:
- onflow/atree#322
- onflow/atree#323
- onflow/atree#324
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.

3 participants