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

docs: RFC-04 Accounts #15507

Merged
merged 20 commits into from
Jul 24, 2023
Merged

docs: RFC-04 Accounts #15507

merged 20 commits into from
Jul 24, 2023

Conversation

testinginprod
Copy link
Contributor

@testinginprod testinginprod commented Mar 22, 2023

Description

Closes: #XXXX

Rendered: https://github.com/cosmos/cosmos-sdk/blob/tip/accounts-rfc/docs/rfc/rfc-004-accounts.md


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@testinginprod testinginprod requested a review from a team as a code owner March 22, 2023 16:28
@tac0turtle tac0turtle changed the title docs: RFC-04 Accounts and Authentication docs: RFC-05 Accounts and Authentication Mar 23, 2023
- Information regarding the sender.
- ...

#### Init
Copy link
Member

Choose a reason for hiding this comment

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

on the last call we talked about how an account will not be automatically instantiated, can we touch on this and the implications?

If a user sends funds from an exchange to the wallet, the exchange would need to support creating the account.

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

So in my mind, what is proposed for accounts and what is proposed for authn are two mostly orthogonal things. I'm not following the logic for why they are coupled here. In particular, I don't see a scenario where an EOA would receive messages but it seems that creating an EOA would be the same as a module account in terms of having a deploy message. I think this coupling of authentication with the accounts abstraction is not needed and these two should be dealt with separately.

I would prefer to call what is described here as an account as an "account-scoped module". Based on the discussion that we had, it is really a module which can be deployed multiple times with different addresses. I do think there is a use case for this, but I think the API for this needs to be refined. In particular, I don't like the usage of opaque proto bytes in messages. There should be some way to avoid this. Second, I think it would be better to not create a separate more-limited context but reuse the core API service abstractions for things like storage and events (assuming we're still committed to this direction).

Now, as I proposed before, I think we should explore extending this to a first class concept within the SDK rather than a dedicated module. This would look like defining a way for a Msg or query to be scoped and routed to a specific handler based on the "account scope". For example say we defined MsgSend in bank like this:

message MsgSend {
  option (cosmos.msg.v1.signer) = "from";

  string from = 1;
  string to = 2;
  string denom = 3 [(cosmos.msg.v1.account_scope) = true];
  string amount = 4;
}

This would allow us to route the msg to the denom interpreted as an account address. And at that account address we would expect some code to be deployed similar to what's described here that handles MsgSend for this denom. I can see this sort of abstraction useful as a way to handle abstractions of denoms, vesting accounts, and gov/group accounts where the same API is needed but slightly different behaviors are implemented. For an account-scoped module defined in this way, the protobuf API becomes a bit more like say the ERC-20 spec - one API, different code.

My reservations around this approach generally are that it makes it harder to coordinate event and schema uniformity across different implementations, but I think we can figure out at least some best practices to address that.

docs/rfc/rfc-005-accounts-auth.md Outdated Show resolved Hide resolved
@testinginprod
Copy link
Contributor Author

testinginprod commented Apr 3, 2023

I think this coupling of authentication with the accounts abstraction is not needed and these two should be dealt with separately.

They're coupled here to show how the process of creating authenticated accounts would look like. Also the current scope of work for authn which only aims to abstract public keys and not the authentication mechanism as a whole, I think a refactor should be sufficient.

I don't see a scenario where an EOA would receive messages

The RFC highlights an example of an account whose credentials can be recovered by another identity. The recoverer would be sending a message to the account that needs to be recovered and ask for a credential change.
I think in general any state change on a account scoped module will require a message to be sent to it.

Now, as I proposed before, I think we should explore extending this to a first class concept within the SDK rather than a dedicated module.

Agreed on moving this to be a first class citizen, I would also say a module would be required to be able to deploy these form of accounts arbitrarily, otherwise every module would need a custom deploy path (message) for creating account scoped modules (EG: bank smart denoms as you have described)

@tac0turtle
Copy link
Member

@testinginprod @aaronc were you two able to catch up?

@aaronc
Copy link
Member

aaronc commented Apr 10, 2023

@testinginprod @aaronc were you two able to catch up?

Not in detail yet but we should

Copy link
Member

@aaronc aaronc 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 think the x/authn and x/accounts proposals should be discussed separately. I would propose breaking this out into two RFCs. I see them as quite orthogonal and I think it would be easier to make progress on each if we separate them out.

@testinginprod testinginprod changed the title docs: RFC-05 Accounts and Authentication docs: RFC-04 Accounts May 9, 2023
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

  1. I think this design needs to be studied more in relationship to the proposed use cases. In discussions on slack, it appears there is a huge amount of overlap between the existing x/authz and x/group modules in terms of use cases and I don't think there's a conclusion that this design is the best solution for whatever features may be lacking in those modules. As I said, I do like the idea of an abstraction like this, but we need to make sure it's the right choice for the use cases we're considering if we're presenting it as such.
  2. I think this RFC should be renamed. This is not a generic account abstraction as I see it, but rather one way to create a "micro-module" or what I've called an "account-scoped module". It could be used to create an account-like abstraction but it could be used for other things and there would still be other ways to create "accounts".

docs/rfc/rfc-004-accounts.md Outdated Show resolved Hide resolved
The current implementation of accounts in the Cosmos SDK is limiting in terms of functionality, extensibility, and overall
architecture. This RFC aims to address the following issues with the current account system:

### 1. Accounts Representation and Authentication Mechanism
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I wouldn't number headers

docs/rfc/rfc-004-accounts.md Show resolved Hide resolved
### Rethinking Account Representation and Business Logic

Instead of representing accounts as simple `google.Protobuf.Any` structures stored in state with no business logi
attached, this proposal suggests a more sophisticated account representation that is closer to module entities.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Define what "module entities" are prior to comparing something to them.

docs/rfc/rfc-004-accounts.md Outdated Show resolved Hide resolved
docs/rfc/rfc-004-accounts.md Show resolved Hide resolved
docs/rfc/rfc-004-accounts.md Show resolved Hide resolved
docs/rfc/rfc-004-accounts.md Outdated Show resolved Hide resolved
docs/rfc/rfc-004-accounts.md Outdated Show resolved Hide resolved
which contains all the information and business logic needed to operate the account.

```go
type Schema struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not immediately clear for what and why this is needed. Also, none of these types are defined from what I can tell.

Copy link
Member

Choose a reason for hiding this comment

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

@testinginprod could you shed some light on this?

Copy link
Contributor Author

@testinginprod testinginprod Jul 17, 2023

Choose a reason for hiding this comment

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

Schema is mainly for reflection of messages and state, this will facilitate wallets (for what messages an account can execute/accept) and explorers (in order to easily discover an account's state).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will also be used to understand if an account adheres to a certain CIP interface, for example account 1234 is a CIP-AbstractedAccount if it accepts MsgAuthenticate message.

testinginprod and others added 3 commits May 10, 2023 11:42
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
when interacting with an account from another account or module, for example: an account is an `authz-interface` account if
it has the following message in its execution messages `MsgProxyStateTransition{ state_transition: google.Protobuf.Any }`.

### Migrate
Copy link
Member

Choose a reason for hiding this comment

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

Would users need to call this or do you see it being automated

newly created to its own account, although there might be use-cases for which the deployer is different from the account
that needs to be linked, in this case a handshake protocol on linking would need to be defined.

#### Predictable address creation
Copy link
Contributor

@nicolaslara nicolaslara May 29, 2023

Choose a reason for hiding this comment

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

We can have predictable addresses be stateless by basing them on hash(account_kind | account_definition) where the kind is the registered account implementation as defined in this document, and account_definition a serialization of what that implementation specifies as its unique-account definition.

Some examples:

  • kind=priv/pub_key_accoount, definition=(pubkey: bytes)
  • kind=cosmwasm_account, definition=(contract_addr: string)
  • kind=zk_bearer_account, definition=(proof: string)
  • kind=module_account, definition=(module_name: string)
  • kind=ibc_account, definition=(channel: string, port: string, sender: string)

Note that in many of these the definition has the same type (a string), so the names on the definition matter. So we can't use protobuf serialization for this. If a specific account type has collisions, then they can include a nonce in their definition.


```

#### MsgDeploy
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like MsgDeploy to be optional. I can see how this is needed for stateful accounts, but many account implementations can be stateless (i.e.: priv/pubkeys). I think there is value in allowing users to interact with accounts that haven't been "deployed".

If the only objective of an account deploy is to make it easy to check its existence (someone has interacted with it), then we can ensure deploy gets called on first interaction if the account doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolaslara MsgDeploy is needed to provide the initial state for the account, in the same way as MsgInstantiate works in cosmwasm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess what I want here is to default state to be assumed for an account if it hasn't been deployed. I think there's value in interacting with inexistent accounts the way it can be done now. This gets more complex with account rekeying though

Copy link
Member

Choose a reason for hiding this comment

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

the auth module will still exist, i think we are leaning towards x/accounts has explicit avvount creation while x/auth has implicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if you want to do an IBC transfer but use the new account type's features you'd have to use x/auth and then migrate right?

For composability (on ibc-hooks, for example), one thing that we can do is to deploy an x/accounts account as part of what gets executed OnRecv. What we're currently doing on ibc-hooks is sending the tokens to sha(prefix|channel|counterparyAccount) and that can only be controlled via ibc-hooks. That could be modified to deploy an account with credentials such that it can be controlled via ibc hooks on that channel or the pubkey of the original account.

Copy link
Member

Choose a reason for hiding this comment

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

So if you want to do an IBC transfer but use the new account type's features you'd have to use x/auth and then migrate right?

if the account doesnt exist most likely.

interesting solution, i didnt think of this. That is pretty good, im not sure who would implement this part though

Copy link
Member

Choose a reason for hiding this comment

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

resolving this conversation as we discussed how app devs could have hooks to create account on the fly for a little extra gas

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

approving this as discussed in the working group call, we should resolve the nits and merge this

@tac0turtle
Copy link
Member

@testinginprod do we need to add any hooks to the base interfaces or are app devs free to do it and dont need anything native

Comment on lines +5 to +6
* 17/03/2023: DRAFT
* 09/05/2023: DRAFT 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest Y-M-D for the date format.

@tac0turtle tac0turtle added this pull request to the merge queue Jul 24, 2023
Merged via the queue into main with commit 4283aec Jul 24, 2023
42 checks passed
@tac0turtle tac0turtle deleted the tip/accounts-rfc branch July 24, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants