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

add a clear method to delete saved data in storage #1253

Closed
wants to merge 3 commits into from
Closed

Conversation

dsainati1
Copy link
Contributor

Closes #1245

Adds a new method to the AuthAccount object:

fun clear(_ path: Path): Bool

This function deletes whatever is present at the specified path, destroying any resources, and returns whether anything was present originally.


  • 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

@codecov-commenter
Copy link

Codecov Report

Merging #1253 (2ee3fb9) into master (81c63bf) will increase coverage by 0.01%.
The diff coverage is 94.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1253      +/-   ##
==========================================
+ Coverage   77.60%   77.61%   +0.01%     
==========================================
  Files         274      274              
  Lines       35233    35261      +28     
==========================================
+ Hits        27341    27369      +28     
  Misses       6805     6805              
  Partials     1087     1087              
Flag Coverage Δ
unittests 77.61% <94.59%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
runtime/interpreter/interpreter.go 89.19% <89.47%> (+0.06%) ⬆️
runtime/interpreter/account.go 93.16% <100.00%> (+0.17%) ⬆️
runtime/sema/authaccount_type.go 100.00% <100.00%> (ø)

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 81c63bf...2ee3fb9. Read the comment docs.

@github-actions
Copy link

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 81c63bf
The command go test ./... -run=XXX -bench=. -shuffle=on -count N was used.
Bench tests were run a total of 7 times on each branch.

Results

old.txtnew.txt
time/opdelta
QualifiedIdentifierCreation/One_level-23.17ns ± 4%3.30ns ± 2%+4.14%(p=0.008 n=7+6)
ParseArray-222.6ms ± 1%23.1ms ± 2%+1.88%(p=0.005 n=6+7)
RuntimeStorageWriteCached-2169µs ±43%143µs ± 2%~(p=0.731 n=7+6)
RuntimeResourceDictionaryValues-216.7ms ± 3%16.4ms ± 3%~(p=0.534 n=7+6)
RuntimeFungibleTokenTransfer-21.24ms ± 4%1.24ms ± 3%~(p=0.445 n=7+6)
ParseFungibleToken-2469µs ± 2%475µs ± 2%~(p=0.165 n=7+7)
ParseInfix-224.6µs ± 2%24.9µs ± 4%~(p=0.456 n=7+7)
ParseDeploy/decode_hex-21.44ms ± 4%1.43ms ± 3%~(p=1.000 n=7+7)
QualifiedIdentifierCreation/Three_levels-2161ns ± 2%163ns ± 4%~(p=0.386 n=6+7)
CheckContractInterfaceFungibleTokenConformance-2168µs ± 3%171µs ± 5%~(p=0.128 n=7+7)
ContractInterfaceFungibleToken-247.4µs ± 1%47.8µs ± 2%~(p=0.318 n=7+7)
InterpretRecursionFib-22.71ms ± 3%2.68ms ± 3%~(p=0.945 n=7+6)
NewInterpreter/new_interpreter-21.16µs ± 5%1.16µs ± 2%~(p=0.781 n=7+7)
NewInterpreter/new_sub-interpreter-22.11µs ± 3%2.10µs ± 3%~(p=0.779 n=7+7)
ParseDeploy/byte_array-235.0ms ± 3%34.3ms ± 2%−2.15%(p=0.038 n=7+7)
 
alloc/opdelta
RuntimeFungibleTokenTransfer-2233kB ± 0%234kB ± 0%+0.40%(p=0.001 n=7+7)
RuntimeResourceDictionaryValues-24.34MB ± 0%4.34MB ± 0%+0.02%(p=0.001 n=7+7)
RuntimeStorageWriteCached-283.7kB ± 0%83.7kB ± 0%~(p=0.294 n=6+7)
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-266.4kB ± 0%66.4kB ± 0%~(p=1.000 n=7+7)
ContractInterfaceFungibleToken-226.7kB ± 0%26.7kB ± 0%~(all equal)
InterpretRecursionFib-21.21MB ± 0%1.21MB ± 0%~(p=0.385 n=7+6)
NewInterpreter/new_interpreter-2680B ± 0%680B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-21.06kB ± 0%1.06kB ± 0%~(all equal)
 
allocs/opdelta
RuntimeFungibleTokenTransfer-24.42k ± 0%4.43k ± 0%+0.06%(p=0.001 n=6+7)
RuntimeResourceDictionaryValues-2108k ± 0%108k ± 0%+0.00%(p=0.005 n=7+7)
RuntimeStorageWriteCached-21.42k ± 0%1.42k ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-21.07k ± 0%1.07k ± 0%~(all equal)
ContractInterfaceFungibleToken-2458 ± 0%458 ± 0%~(all equal)
InterpretRecursionFib-225.0k ± 0%25.0k ± 0%~(all equal)
NewInterpreter/new_interpreter-211.0 ± 0%11.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-231.0 ± 0%31.0 ± 0%~(all equal)
 

@turbolent
Copy link
Member

@dete Any thoughts?

@turbolent
Copy link
Member

@dsainati1 Should we maybe consider this change for Stable Cadence?

@dsainati1
Copy link
Contributor Author

@dsainati1 Should we maybe consider this change for Stable Cadence?

Seems to me like based on the discussion on #1252, the sentiment is against these two methods, the argument being that any operation that destroys a resource should be explicit; having methods that can implicitly destroy resources is not a desirable feature for the language.

However I would be willing to put this to a FLIP if it's something we want to push for anyways.

@turbolent
Copy link
Member

@dsainati1 Sounds good! Added it to the agenda of the next LDM, maybe we can decide there together if we want to close this and the other related PR, or open a FLIP for them

@turbolent
Copy link
Member

Put on-hold for now, see https://github.com/onflow/cadence/blob/master/meetings/2022-10-11.md#additional-storage-api-functions

@dsainati1
Copy link
Contributor Author

Seems like this feature is not wanted; closing.

@dsainati1 dsainati1 closed this Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a clear function to erase stored data
3 participants