add `forceSave` function to save and overwrite stored data

[dsainati1]

Closes #1244

Description

Adds the following method to AuthAccount objects:

fun forceSave<T>(_ value: T, to path: Path): Bool

This will save a value to the specified path, overwriting any data present (and destroying it if it is a resource). Returns whether or not any data was overwritten.

---

• [X] Targeted PR against master branch
• [X] Linked to Github issue with discussion and accepted design OR link to spec that describes this work
• [X] Code follows the standards mentioned here
• [X] Updated relevant documentation
• [X] Re-reviewed Files changed in the Github PR explorer
• [X] Added appropriate labels

[github-actions[bot]]

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 81c63bfef73cd4874d05b6274dbd2402a0a0b788 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.txt	new.txt
	time/op		delta
ParseInfix-2	27.2µs ± 3%	28.0µs ± 3%	+2.66%	(p=0.038 n=7+7)
InterpretRecursionFib-2	2.92ms ± 1%	2.99ms ± 4%	+2.45%	(p=0.035 n=6+7)
RuntimeFungibleTokenTransfer-2	1.44ms ±32%	1.33ms ± 5%	~	(p=1.000 n=7+7)
RuntimeResourceDictionaryValues-2	17.9ms ± 4%	17.5ms ± 3%	~	(p=0.073 n=7+7)
RuntimeStorageWriteCached-2	153µs ± 4%	186µs ±35%	~	(p=0.620 n=7+7)
ParseArray-2	25.4ms ± 2%	26.2ms ± 4%	~	(p=0.073 n=7+7)
ParseFungibleToken-2	527µs ± 2%	526µs ± 3%	~	(p=0.805 n=7+7)
ParseDeploy/decode_hex-2	1.61ms ± 1%	1.58ms ± 3%	~	(p=0.053 n=7+7)
QualifiedIdentifierCreation/One_level-2	3.54ns ± 1%	3.63ns ± 4%	~	(p=0.051 n=6+7)
CheckContractInterfaceFungibleTokenConformance-2	182µs ± 5%	181µs ± 2%	~	(p=0.628 n=7+6)
ContractInterfaceFungibleToken-2	51.2µs ± 1%	51.8µs ± 4%	~	(p=0.534 n=6+7)
NewInterpreter/new_interpreter-2	1.26µs ± 3%	1.26µs ± 1%	~	(p=1.000 n=7+6)
NewInterpreter/new_sub-interpreter-2	2.28µs ± 1%	2.30µs ± 4%	~	(p=0.639 n=5+7)
ParseDeploy/byte_array-2	39.1ms ± 1%	38.4ms ± 1%	−1.71%	(p=0.002 n=6+6)
QualifiedIdentifierCreation/Three_levels-2	178ns ± 2%	175ns ± 3%	−1.77%	(p=0.026 n=7+7)
	alloc/op		delta
RuntimeFungibleTokenTransfer-2	233kB ± 0%	234kB ± 0%	+0.40%	(p=0.001 n=7+7)
RuntimeResourceDictionaryValues-2	4.34MB ± 0%	4.34MB ± 0%	+0.02%	(p=0.001 n=7+7)
RuntimeStorageWriteCached-2	83.7kB ± 0%	83.7kB ± 0%	~	(p=1.000 n=6+6)
QualifiedIdentifierCreation/One_level-2	0.00B	0.00B	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	64.0B ± 0%	64.0B ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	66.4kB ± 0%	66.4kB ± 0%	~	(p=0.462 n=7+7)
ContractInterfaceFungibleToken-2	26.7kB ± 0%	26.7kB ± 0%	~	(all equal)
InterpretRecursionFib-2	1.21MB ± 0%	1.21MB ± 0%	~	(p=1.000 n=7+6)
NewInterpreter/new_interpreter-2	680B ± 0%	680B ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	1.06kB ± 0%	1.06kB ± 0%	~	(all equal)
	allocs/op		delta
RuntimeFungibleTokenTransfer-2	4.42k ± 0%	4.43k ± 0%	+0.05%	(p=0.001 n=7+6)
RuntimeResourceDictionaryValues-2	108k ± 0%	108k ± 0%	+0.00%	(p=0.004 n=7+7)
RuntimeStorageWriteCached-2	1.42k ± 0%	1.42k ± 0%	~	(all equal)
QualifiedIdentifierCreation/One_level-2	0.00	0.00	~	(all equal)
QualifiedIdentifierCreation/Three_levels-2	2.00 ± 0%	2.00 ± 0%	~	(all equal)
CheckContractInterfaceFungibleTokenConformance-2	1.07k ± 0%	1.07k ± 0%	~	(all equal)
ContractInterfaceFungibleToken-2	458 ± 0%	458 ± 0%	~	(all equal)
InterpretRecursionFib-2	25.0k ± 0%	25.0k ± 0%	~	(all equal)
NewInterpreter/new_interpreter-2	11.0 ± 0%	11.0 ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	31.0 ± 0%	31.0 ± 0%	~	(all equal)

[codecov-commenter]

Codecov Report

Merging #1252 (b8b1d5b) into master (81c63bf) will increase coverage by 0.03%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1252      +/-   ##
==========================================
+ Coverage   77.60%   77.63%   +0.03%     
==========================================
  Files         274      274              
  Lines       35233    35281      +48     
==========================================
+ Hits        27341    27389      +48     
  Misses       6805     6805              
  Partials     1087     1087              

Flag	Coverage Δ
unittests	77.63% <100.00%> (+0.03%)	:arrow_up:

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

Impacted Files	Coverage Δ
runtime/interpreter/account.go	93.16% <100.00%> (+0.17%)	:arrow_up:
runtime/interpreter/interpreter.go	89.23% <100.00%> (+0.09%)	:arrow_up:
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...b8b1d5b. Read the comment docs.

[joshuahannan]

Is it safe to add stuff like this and the clear method? It might encourage people to use bad practices for handling resources

[dsainati1]

forceSave and clear are both safe with respect to resources because they will destroy any resources that are potentially saved at the destination. Neither method really adds any new functionality to the language; they only make certain patterns easier.

account.clear(/storage/x)

is equivalent to

let r <- account.load<@R>(/storage/x)
destroy r

while

account.forceSave(<-r, /storage/x)

is equivalent to

account.clear(/storage/x)
account.save(<-r, /storage/x)

[joshuahannan]

I understand that they are just simpler patters for things that already exist, but I feel like this kind of blurs the line on the rule "resources cannot be lost, they have to be explicitly destroyed". To me, this is not explicit destruction and kind of breaks that rule, and I could see some people reverting to using these by default instead of the more explicit versions that we currently use, which could cause unintended loss of resources in some circumstances.

I would support adding these for non-resource types, but it makes me nervous to add them for resource types

[bluesign]

I agree with @joshuahannan, when you give developer hammer, everything seems like nail to them, for example: when setting up FUSD Vault imagine developer accidentally override.

I think storage system ( cadence part ) needs a rethinking, instead of adding features. Those changes are not solving any serious problem, like storage sandboxing etc...

I was expecting after storage improvements, there would be some work on this area. IMO at least we need:

• sandboxing ( dapp isolation )
• capability revoke
• loading types which does not exist anymore

If it was up to me, I would even get rid of Paths totally.

[turbolent]

@dete Any thoughts here?

[turbolent]

@bluesign Fair enough on the feedback on this PR in particular, but this PR isn't the right place to vent your expectations about the roadmap. Please keep the conversation productive and focused on this PR in particular. Thanks

[bluesign]

@bluesign Fair enough on the feedback on this PR in particular, but this PR isn't the right place to vent your expectations about the roadmap. Please keep the conversation productive and focused on this PR in particular. Thanks

sorry about that, I went off topic a bit. :(

[turbolent]

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

[dsainati1]

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

See comment on https://github.com/onflow/cadence/pull/1253

[turbolent]

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

[dsainati1]

Seems like this feature is not wanted; closing.