Allow scripts to access authorized accounts #1240

dsainati1 · 2021-11-16T19:10:06Z

Closes #539

Description

Because scripts discard their changes upon completion, there is no harm in allowing them to access AuthAccounts. This adds a function:

getAuthAccount(_ address: Address): AuthAccount

That loads an AuthAccount in the same way that getAccount loads a PublicAccount. This is only available in scripts; trying to use this function outside of a script will cause an error in the checker.

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

…nments

github-actions · 2021-11-16T19:21:54Z

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit ad6895c
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
NewInterpreter/new_interpreter-2	1.16µs ± 2%	1.21µs ± 5%	+4.60%	(p=0.001 n=6+7)
ParseDeploy/byte_array-2	34.5ms ± 2%	35.9ms ± 3%	+4.06%	(p=0.001 n=7+7)
ParseFungibleToken-2	474µs ± 3%	489µs ± 2%	+3.24%	(p=0.007 n=7+7)
ParseDeploy/decode_hex-2	1.44ms ± 2%	1.48ms ± 2%	+3.11%	(p=0.001 n=7+7)
RuntimeStorageWriteCached-2	170µs ±39%	142µs ± 1%	~	(p=0.209 n=7+7)
RuntimeResourceDictionaryValues-2	16.4ms ± 3%	16.3ms ± 3%	~	(p=0.902 n=7+7)
RuntimeFungibleTokenTransfer-2	1.20ms ± 4%	1.23ms ± 4%	~	(p=0.259 n=7+7)
ParseInfix-2	25.1µs ± 2%	25.1µs ± 2%	~	(p=0.932 n=7+7)
ParseArray-2	23.4ms ± 1%	23.6ms ± 3%	~	(p=0.234 n=6+7)
QualifiedIdentifierCreation/One_level-2	3.26ns ± 3%	3.26ns ± 2%	~	(p=0.902 n=7+7)
QualifiedIdentifierCreation/Three_levels-2	163ns ± 2%	163ns ± 3%	~	(p=0.902 n=7+7)
CheckContractInterfaceFungibleTokenConformance-2	171µs ± 3%	170µs ± 1%	~	(p=0.937 n=6+6)
ContractInterfaceFungibleToken-2	47.6µs ± 4%	48.1µs ± 3%	~	(p=0.165 n=7+7)
NewInterpreter/new_sub-interpreter-2	2.14µs ± 2%	2.18µs ± 3%	~	(p=0.154 n=7+7)
InterpretRecursionFib-2	2.74ms ± 4%	2.73ms ± 2%	~	(p=1.000 n=7+7)

	alloc/op		delta
RuntimeFungibleTokenTransfer-2	233kB ± 0%	233kB ± 0%	+0.01%	(p=0.025 n=7+7)
RuntimeStorageWriteCached-2	83.7kB ± 0%	83.7kB ± 0%	~	(p=1.000 n=6+6)
RuntimeResourceDictionaryValues-2	4.34MB ± 0%	4.34MB ± 0%	~	(p=0.318 n=7+7)
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)
NewInterpreter/new_interpreter-2	680B ± 0%	680B ± 0%	~	(all equal)
NewInterpreter/new_sub-interpreter-2	1.06kB ± 0%	1.06kB ± 0%	~	(all equal)
InterpretRecursionFib-2	1.21MB ± 0%	1.21MB ± 0%	~	(p=0.462 n=7+7)

	allocs/op		delta
RuntimeStorageWriteCached-2	1.42k ± 0%	1.42k ± 0%	~	(all equal)
RuntimeResourceDictionaryValues-2	108k ± 0%	108k ± 0%	~	(p=0.264 n=7+7)
RuntimeFungibleTokenTransfer-2	4.43k ± 0%	4.43k ± 0%	~	(p=1.000 n=7+7)
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)
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)
InterpretRecursionFib-2	25.0k ± 0%	25.0k ± 0%	~	(all equal)

joshuahannan

Exciting! 🎉 🥳

docs/language/accounts.mdx

codecov-commenter · 2021-11-16T19:45:04Z

Codecov Report

Merging #1240 (a05e443) into master (ad6895c) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head a05e443 differs from pull request most recent head 452d43e. Consider uploading reports for the commit 452d43e to get more accurate results

@@            Coverage Diff             @@
##           master    #1240      +/-   ##
==========================================
+ Coverage   77.59%   77.61%   +0.01%     
==========================================
  Files         274      274              
  Lines       35232    35260      +28     
==========================================
+ Hits        27339    27367      +28     
  Misses       6806     6806              
  Partials     1087     1087

Flag	Coverage Δ
unittests	`77.61% <100.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
runtime/runtime.go	`86.96% <100.00%> (+0.20%)`	⬆️

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 ad6895c...452d43e. Read the comment docs.

turbolent

Great work!

runtime/cmd/parse/main.go

runtime/runtime.go

bluesign · 2021-11-19T11:03:28Z

Why not extend PublicAccount with functions from AuthAccount in script context ?

Co-authored-by: Bastian Müller <[email protected]>

dsainati1 · 2021-11-19T15:13:31Z

Why not extend PublicAccount with functions from AuthAccount in script context ?

Having the same type mean different things in different contexts is confusing; we don't want to change what a PublicAccount means depending on where a script is being used. Instead, it's a much simpler change to allow people to access AuthAccounts depending on whether they are in a script or a transaction, and this way each of the types mean the same thing everywhere. The only thing changing is whether they are accessible.

bluesign · 2021-11-19T20:18:57Z

I know it is already merged, but here is my 2 cents.

This is the diff of AuthAccount from PublicAccount:

struct AuthAccount {
    fun addPublicKey(_ publicKey: [UInt8])
    fun removePublicKey(_ index: Int)
    fun save<T>(_ value: T, to: StoragePath)
    fun load<T>(from: StoragePath): T?
    fun copy<T: AnyStruct>(from: StoragePath): T?
    fun borrow<T: &Any>(from: StoragePath): T?
    fun getCapability<T>(_ path: CapabilityPath): Capability<T>
    fun link<T: &Any>(_ newCapabilityPath: CapabilityPath, target: Path): Capability<T>?
    fun unlink(_ path: CapabilityPath)
    struct Contracts {
        fun add(
            name: String,
            code: [UInt8],
            ... contractInitializerArguments
        ): DeployedContract
        fun update__experimental(name: String, code: [UInt8]): DeployedContract
        fun remove(name: String): DeployedContract?
    }
    struct Keys {
        fun add(
            publicKey: PublicKey,
            hashAlgorithm: HashAlgorithm,
            weight: UFix64
        ): AccountKey
        fun revoke(keyIndex: Int): AccountKey?
    }
}

Below is the function that makes sense in script ( read only ): (considering getCapability is useless)

struct AuthAccount {
    fun load<T>(from: StoragePath): T?
    fun copy<T: AnyStruct>(from: StoragePath): T?
    fun borrow<T: &Any>(from: StoragePath): T?
}

technically load and copy are same (in script context) borrow is useless when you have load. So we end up with:

fun load<T>(from: StoragePath): T?

In my opinion exposing 14 functions where in reality 1 is needed outweights the benefit. Also PublicAccount has feeling of read-only vs AuthAccount has feeling of read/write.

instead of getAuthAccount (which is exposed to scripts only), maybe it would be better to have something like:

fun loadFromAccount<T>(Address, path: StoragePath): T?

But I think it would also make sense to just put load to PublicAccount. And just panic with a nice error message when in Transaction context.

turbolent · 2022-03-01T20:24:11Z

@bluesign The difference between a transaction context and a script context is that in a transaction context state changes are written, and in the script context the changes are ignored.

It is much simpler for the implementation to behave exactly the same in both contexts and I would assume that it is also the same for developers/users. I don't really think that changing the definitions and APIs (e.g. adding new / removing existing functions) is worth it.

Having the existing definitions behave the same in a scripting context as in a transaction context is also useful for future features like testing, where you might want to perform state-changing actions, but are not interested in the state changes being applied. If the API were reduced to just a "read-only" API it would be limiting for some actions you might want to perform.

bluesign · 2022-03-02T10:25:23Z

Having the existing definitions behave the same in a scripting context as in a transaction context is also useful for future features like testing, where you might want to perform state-changing actions, but are not interested in the state changes being applied. If the API were reduced to just a "read-only" API it would be limiting for some actions you might want to perform.

Oh thats exactly the point that I missed, I now see how this can be a powerful feature. Thanks for the clarification.

bluesign · 2022-05-20T11:50:36Z

Any eta on this to be included in Cadence ?

dsainati1 · 2022-05-20T14:02:18Z

This will be out in Secure Cadence I believe

dsainati1 added 3 commits November 15, 2021 14:41

conditionally add getAuthAccount function to checker in script enviro…

a0fff4b

…nments

Merge branch 'master' of github.com:onflow/cadence into issue539

ca872b9

refactor implementation to runtime.go

e6c8398

dsainati1 added the Feature label Nov 16, 2021

dsainati1 requested review from SupunS and turbolent as code owners November 16, 2021 19:10

update docs

fb3ecec

dsainati1 requested a review from joshuahannan as a code owner November 16, 2021 19:13

dsainati1 self-assigned this Nov 16, 2021

joshuahannan reviewed Nov 16, 2021

View reviewed changes

docs/language/accounts.mdx Show resolved Hide resolved

dsainati1 added 2 commits November 16, 2021 14:44

improve documentation

3d15cdc

fix typo

a05e443

turbolent approved these changes Nov 19, 2021

View reviewed changes

runtime/cmd/parse/main.go Outdated Show resolved Hide resolved

runtime/runtime.go Outdated Show resolved Hide resolved

Update runtime/cmd/parse/main.go

452d43e

Co-authored-by: Bastian Müller <[email protected]>

refactor value into global

1d11932

dsainati1 merged commit 2ea862a into master Nov 19, 2021

turbolent mentioned this pull request Nov 19, 2021

Auto Cadence Update onflow/flow#697

Merged

dsainati1 deleted the issue539 branch November 19, 2021 16:34

turbolent mentioned this pull request Nov 19, 2021

Auto Cadence Update: Allow scripts to access authorized accounts onflow/flow-go#1647

Closed

SupunS mentioned this pull request Dec 20, 2021

Documentation incorrectly suggests that getAuthAccount is supported #1321

Closed

bluesign mentioned this pull request May 20, 2022

Account storage inspection onflow/flow-emulator#151

Closed

turbolent mentioned this pull request May 20, 2022

Add test cases for account mutation in scripts onflow/flow-go#2464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow scripts to access authorized accounts #1240

Allow scripts to access authorized accounts #1240

dsainati1 commented Nov 16, 2021 •

edited

Loading

github-actions bot commented Nov 16, 2021 •

edited

Loading

joshuahannan left a comment

codecov-commenter commented Nov 16, 2021 •

edited

Loading

turbolent left a comment

bluesign commented Nov 19, 2021

dsainati1 commented Nov 19, 2021

bluesign commented Nov 19, 2021

turbolent commented Mar 1, 2022

bluesign commented Mar 2, 2022

bluesign commented May 20, 2022

dsainati1 commented May 20, 2022

Allow scripts to access authorized accounts #1240

Allow scripts to access authorized accounts #1240

Conversation

dsainati1 commented Nov 16, 2021 • edited Loading

Description

github-actions bot commented Nov 16, 2021 • edited Loading

Cadence Benchstat comparison

Results

joshuahannan left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 16, 2021 • edited Loading

Codecov Report

turbolent left a comment

Choose a reason for hiding this comment

bluesign commented Nov 19, 2021

dsainati1 commented Nov 19, 2021

bluesign commented Nov 19, 2021

turbolent commented Mar 1, 2022

bluesign commented Mar 2, 2022

bluesign commented May 20, 2022

dsainati1 commented May 20, 2022

dsainati1 commented Nov 16, 2021 •

edited

Loading

github-actions bot commented Nov 16, 2021 •

edited

Loading

codecov-commenter commented Nov 16, 2021 •

edited

Loading