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

FLIP - AuthAccount Capabilities Management Standard #72

Merged
merged 20 commits into from
Aug 15, 2023

Conversation

sisyphusSmiling
Copy link
Contributor

Forum Post: https://forum.onflow.org/t/account-linking-authaccount-capabilities-management/4314

This FLIP introduces a proposed standard for how AuthAccount Capabilities are managed and brings into discussion a number of open questions around the topic of account linking.

@sisyphusSmiling sisyphusSmiling added the flip: application Application FLIP label Feb 23, 2023
@sisyphusSmiling sisyphusSmiling self-assigned this Feb 23, 2023
@sisyphusSmiling sisyphusSmiling force-pushed the sisyphusSmiling/auth-account-cap-management branch 3 times, most recently from 02600cc to 239dd8c Compare February 24, 2023 00:06
@sisyphusSmiling sisyphusSmiling marked this pull request as ready for review February 24, 2023 00:08
@turbolent turbolent requested a review from a team February 24, 2023 05:23
Copy link

@louisguitton louisguitton left a comment

Choose a reason for hiding this comment

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

impressive work 👏 the demo during the hackathon was really interesting and I'll need to find the time to properly read through this. For now I've found this typo.

- [Design Proposal](#design-proposal)
- [Example Implementation](#example-implementation)
- [Considered For Inclusion](#considered-for-inclusion)
- [Standardizing child accounts’ resources access](#standardizing-child-accounts-resources-access)

Choose a reason for hiding this comment

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

Suggested change
- [Standardizing child accounts’ resources access](#standardizing-child-accounts-resources-access)
- [Storage Iteration Convenience Methods](#storage-iteration-convenience-methods)

pub event ChildAccountRemoved(parent: Address, child: Address)
```

- **Account Creation** - Emitted when an account is created from either the `ChildAccountManager` or `ChildAccountCreator` `createChildAccount()` methods. Since this method takes the arguments `signer: AuthAccount`, `initialFundingAmount: UFix64`, and `childAccountInfo: ChildAccountInfo`, there is room to include more information that may be relevant to callers, namely values from the `childAccountInfo` metadata struct.
Copy link

@highskore highskore Mar 8, 2023

Choose a reason for hiding this comment

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

I think having the originating publicKey as one of the params in the Account Creation events would be very beneficial.

Choose a reason for hiding this comment

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

Something like

pub event ChildAccountCreatedFromManager(parent: Address, child: Address, originatingPublicKey: String)
pub event AccountCreatedFromCreator(creator: Address?, newAccount: Address, originatingPublicKey: String)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good suggestion. I believe you raised the issue in the Open House yesterday - would this event data allow dApps to determine the relevant creation event?

Choose a reason for hiding this comment

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

Yea I did raise it yesterday, I think a combination of the creator address, which you already specified, and the originatingPublicKey would allow dApps to determine the events that they care about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks for the helpful feedback! I'll add this now

///
pub resource ChildAccountCreator : ChildAccountCreatorPublic {
/// mapping of public_key: address
access(self) let createdChildren: {String: Address}
Copy link

@highskore highskore Mar 9, 2023

Choose a reason for hiding this comment

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

Isn't this a cadence anti-pattern?

Is this field really necessary if people can already query the events to find out which account has created which children?

I can imagine after a dApp creates a big number of accounts, this dictionary will not be able to take new inputs. So we would need this to be sharded or it's not really scalable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is definitely a concern. It served as an intermittent solution for the walletless demo, but we weren't sure if it would be helpful long-term. Since the inclusion of the Creator is an open question and not critical to account linking, this detail might imply we remove it from the standard.

Copy link
Member

Choose a reason for hiding this comment

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

Since the parent accounts are managing entire accounts, then I don't think that it is wrong to store a list of the addresses it controls since it does literally control the entire address and account addresses can't be changed

Choose a reason for hiding this comment

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

@sisyphusSmiling Right, that's very true, imo it would be a much cleaner solution if the ChildAccount contract did not include any account creation methods at all, and only have the Child -> Parent account linking features.

Then the dapps/users could handle account creation in their own custom ways and only use the ChildAccount contract for the purpose of linking and managing child(parent) accounts.

@sisyphusSmiling
Copy link
Contributor Author

sisyphusSmiling commented Mar 15, 2023

Quick update on this to everyone following along. Based on the feedback received so far, I'm working to include the following changes in the next iteration of this contract:

My plan is to get this implementation done and reflected in this FLIP by EOD today. If interested, you can follow along in this branch of the onflow/linked-accounts repo

@sisyphusSmiling
Copy link
Contributor Author

Quick update on progress over the week as it's not visible in the contents of this FLIP:

We responded to concerns raised by community developers related to sharing user-controlled access on app-custodied accounts. The proposed solution builds on this FLIP, though works to separate concerns between user and developer side of the house with this FLIP solving the user side of the equation.

Essentially, the proposed solution, outlined in this forum post, enables developers to define restricted access on the linked account which takes effect in the same transaction which links user and app accounts. The user then has unrestricted access while the app retains access to only the Capabilities needed to act on behalf of the user in the context of the dapp.

Moving forward, the focus for this week is increasing test coverage of the implementation contract & supporting transactions to ensure sound security. Additionally, I'll be working to break down concepts related to account linking & hybrid custody that will help devs get up to speed quickly.

Please check out the linked forum post, appreciate it all!

@sisyphusSmiling
Copy link
Contributor Author

sisyphusSmiling commented Apr 24, 2023

Adding another quick update here now that we've aligned on an access model:

After lots of great discussion, the proposal for an unrestricted user/restricted app access model is being flipped in favor of a restricted user/unrestricted app access model. Check out the flowtyio/restricted-child-account repo for work implementation level details and work on this front moving forward.

This is due to a number of factors including:

  • prioritizing ease of app implementation
  • minimizing potential foot guns for app developers
  • interoperability with existing app custodial infrastructure and common practices
  • need to return Typed Capabilities (very cool work with @austinkline's CapabilityFactory to enable this)

You can find the discussion in the previously linked forum post and meeting notes.

Remaining open questions:

Next steps:

  • Update this FLIP with new access model overview
  • Feedback and iteration on HybridCustody contract PR
  • Submit PR with suggested events as conversation point on the issue

@austinkline
Copy link
Contributor

austinkline commented Apr 24, 2023

Thanks for the added summary @sisyphusSmiling! Adding some notes about the current PR as it pertains to outstanding issues

Rules around multi-tenancy (IOW managing multiple parents & rule sharing)

How it works in the active PR is that a ChildAccount resource creates and manages underlying ProxyAccount resources which each have their own rules managed by the account's owner. Parents are shared a capability to a proxy only meant for them. Even if the mechanisms in place are complex, the owner of the child account only needs to know about a few methods to add, revoke, or edit existing parents (edits aren't done yet)

Decide how we want to handle access promotion from restricted -> unrestricted user access

The ChildAccount has a few methods to change an account's ownership which would then alter restricted -> unrestricted access. Mainly, there are two forms of modified ownership:

  1. Transferred Ownership - The current owner of a child account gives ownership of the account to another wallet. This will revoke all outstanding keys on the wallet, unlink all existing AuthAccount capabilities, and make a new one for existing parent/child accounts to keep operating as usual. The new owner gets a capability to the ChildAccount which they can then access as they please. This will give them the full AuthAccount capability which means they have unrestricted access
  2. Relinquished Ownership - Needs a new name, probably. But the idea here is that no one fully owns the account. An unowned account could be jointly shared between two parents but neither can change the access another one has.

Here is a diagram for how the current PR works:
HybridCustody-diagram

@sisyphusSmiling sisyphusSmiling force-pushed the sisyphusSmiling/auth-account-cap-management branch from 42af4a2 to 573028e Compare May 9, 2023 22:18
@sisyphusSmiling sisyphusSmiling force-pushed the sisyphusSmiling/auth-account-cap-management branch from 573028e to f7e9764 Compare May 19, 2023 17:25
@sisyphusSmiling sisyphusSmiling requested review from a team and removed request for franklywatson August 7, 2023 19:03
@sisyphusSmiling
Copy link
Contributor Author

FYI This FLIP has been updated to reflect the current state of HybridCustody contracts currently deployed to Testnet and proposed for Mainnet.

Please take a look and leave your thoughts/reviews.

@sisyphusSmiling sisyphusSmiling removed the request for review from a team August 8, 2023 16:14
///
pub resource ChildAccountCreator : ChildAccountCreatorPublic {
/// mapping of public_key: address
access(self) let createdChildren: {String: Address}
Copy link
Member

Choose a reason for hiding this comment

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

Since the parent accounts are managing entire accounts, then I don't think that it is wrong to store a list of the addresses it controls since it does literally control the entire address and account addresses can't be changed

sisyphusSmiling and others added 19 commits August 15, 2023 15:37
…rd.md

Co-authored-by: Joshua Hannan <joshua.hannan@dapperlabs.com>
…rd.md

Co-authored-by: Joshua Hannan <joshua.hannan@dapperlabs.com>
…rd.md

Co-authored-by: j pimmel <frankly.watson@gmail.com>
…rd.md

Co-authored-by: j pimmel <frankly.watson@gmail.com>
Co-authored-by: j pimmel <frankly.watson@gmail.com>
@sisyphusSmiling sisyphusSmiling force-pushed the sisyphusSmiling/auth-account-cap-management branch from 2c61f3a to 60c2b5c Compare August 15, 2023 21:38
@sisyphusSmiling sisyphusSmiling force-pushed the sisyphusSmiling/auth-account-cap-management branch from 60c2b5c to c8b3f1f Compare August 15, 2023 21:39
@sisyphusSmiling sisyphusSmiling merged commit c045024 into main Aug 15, 2023
@sisyphusSmiling sisyphusSmiling deleted the sisyphusSmiling/auth-account-cap-management branch August 15, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flip: application Application FLIP
Projects
Status: Drafted
Development

Successfully merging this pull request may close these issues.

7 participants