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

Account type #92

Merged
merged 15 commits into from
Aug 15, 2023
Merged

Account type #92

merged 15 commits into from
Aug 15, 2023

Conversation

turbolent
Copy link
Member

@turbolent turbolent commented May 26, 2023

The goal of this proposal is to improve how Cadence provides access to accounts.

The proposed API aims to make account access simpler and safer, by enabling and encouraging the principle of least authority (PoLA) for account access.

This FLIP is based on previous discussion in https://forum.onflow.org/t/super-user-account/4088

@turbolent turbolent requested a review from a team May 26, 2023 21:49
@turbolent turbolent self-assigned this May 26, 2023
Copy link
Contributor

@bjartek bjartek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this proposal pub is still used? Is that going away or not.

This is a really good example of entilements and exactly the kind I wanted to se. I can ser the power it gives.

@bjartek
Copy link
Contributor

bjartek commented May 27, 2023

What type will an account we in a prepare statement have?

@turbolent
Copy link
Member Author

@bjartek As described in the "Expose Account Access Through References" section, account values will be only exposed through references, i.e. &Account. The Examples section shows an example.

@austinkline
Copy link
Contributor

Thanks for putting this together @turbolent! My main concern as of now is how an AuthAccount stored in a struct/resource would be migrated. For instance, hybrid custody will need to store an AuthAccount capability in a resource which helps manage access for a parent to get to a child account. Since we're having to add this to a type definition, current type restrictions on contract upgrades would prevent us from migrating to this a new set of interfaces

Any ideas how we can get around that issue?

@turbolent
Copy link
Member Author

@austinkline I clarified in the last commit, 002924e, how a &AuthAccount static type (e.g. used in a Capability), would be migrated.

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely feel great about this improvement!

cadence/20230525-account-type.md Outdated Show resolved Hide resolved
cadence/20230525-account-type.md Outdated Show resolved Hide resolved
@austinkline
Copy link
Contributor

Adding another thought as I work through some other hybrid custody related items, what happens if someone has already named an interface or struct of theirs Account in an existing contract?

Copy link
Contributor

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This is both a great improvement to the language and a good demonstrative example of the benefits of entitlements.

One point to consider: not to reopen the bikeshed from #86, but regardless of which part of speech and casing convention we decide upon, we should make sure that they are aligned between these two FLIPs, as these will be the likely extent of the built-in entitlements that Cadence has. Currently this FLIP uses a mix of noun and verb entitlements (we should maybe try to make these more uniform?) while the mutability FLIP uses adjectives.

cadence/20230525-account-type.md Show resolved Hide resolved
cadence/20230525-account-type.md Show resolved Hide resolved
@turbolent
Copy link
Member Author

turbolent commented May 31, 2023

@austinkline

What happens if someone has already named an interface or struct of theirs Account in an existing contract

That would lead to an type-checking error where the Account type is declared.

A quick cursory search in https://github.com/bluesign/flow-mainnet-contracts shows that there are currently no such types on Mainnet (yet).

@austinkline
Copy link
Contributor

@austinkline

What happens if someone has already named an interface or struct of theirs Account in an existing contract

That would lead to an type-checking error where the Account type is declared.

A quick cursory search in https://github.com/bluesign/flow-mainnet-contracts shows that there are currently no such types on Mainnet (yet).

Got it, checking for what's out there was my next question. Sounds like only HybridCustody is an issue (we have an Account type in testnet). I'll go ahead and change it tomorrow just to be safe.

austinkline added a commit to onflow/hybrid-custody that referenced this pull request Jun 1, 2023
The Account type flip found
[here](onflow/flips#92) is set to define a new
type which will cause a conflict in our own type definitions.
AuthAccount is going to be split into a set of interfaces and a concrete
called `Account` which we also define. If the flip were to be
implemented, that type would brick our own contract due to `Account`
being a reserved type/keyword.

Since we aren't in mainnet yet, we can rename the type here and avoid
the conflict entirely.
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this idea!
Being able to remove the #allowAccountLinking pragma is also a nice side effect 👌

cadence/20230525-account-type.md Show resolved Hide resolved
cadence/20230525-account-type.md Show resolved Hide resolved
@bjartek
Copy link
Contributor

bjartek commented Jun 20, 2023

I think the change is good, but i fear the cost of migration here aswell. Is it even possible to do in all cases if we cannot change the type of a field?

@turbolent
Copy link
Member Author

I just realized that we'll also need to rethink getAuthAccount.

Maybe add a type parameter, so any entitled &Account reference can be "summoned"? For example, getAuthAccount<auth(Storage) &Account>(0x1).

We could also "merge" getAuthAccount into getAccount, and have a different type parameter depending on environment, e.g. "hardcode" to &Account in transactions and allow any &Account subtype in scripts.

@turbolent
Copy link
Member Author

@bjartek we'll likely need to somehow allow code to be upgraded in ways that the checker currently rejects, but are actually safe, e.g. here, the name just changes, the underlying value will still be the same (and is actually not storable)

@turbolent
Copy link
Member Author

BTW, there has been an ongoing discussion about naming of entitlements in another FLIP introducing built-in entitlements: #86 (comment)

@turbolent
Copy link
Member Author

turbolent commented Jul 7, 2023

Some issues I ran into while starting the implementation of this FLIP:

  • All contracts have a a predeclared field account, which currently has the type AuthAccount.

    How should we replace this?

    There is intentionally no direct replacement for AuthAccount, so all we can do is to "fully entitle" it as auth(Storage, Contracts, Keys, Inbox, Capabilities) &Account.

  • The constructor AuthAccount will also need to change.

    • It should probably be renamed to just Account?

    • Its payer parameter currently has type AuthAccount.
      Payment at least requires access to storage, so maybe require the Storage entitlement?

      The fine-grained BorrowValue entitlement would probably also suffice, so maybe the entitlement set should be BorrowValue | Storage? cc @dsainati1

@dsainati1
Copy link
Contributor

dsainati1 commented Jul 10, 2023

All contracts have a a predeclared field account, which currently has the type AuthAccount. How should we replace this?

IMO it makes the most sense to just add the fully entitled type here auth(Storage, Contracts, Keys, Inbox, Capabilities) &Account. It's verbose, but users will rarely have to write the full type, and I don't think we should add anything new to the language just for this case.

The fine-grained BorrowValue entitlement would probably also suffice, so maybe the entitlement set should be BorrowValue | Storage?

This makes sense to me.

@turbolent
Copy link
Member Author

Approved in the Cadence Language Design Meeting 🎉

@turbolent turbolent merged commit 2f67479 into main Aug 15, 2023
@turbolent turbolent deleted the bastian/account-type branch August 15, 2023 18:13
@KshitijChaudhary666 KshitijChaudhary666 mentioned this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants