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

authz: if grantee doesn't exist, should create account #10590

Closed
4 tasks
cmwaters opened this issue Nov 22, 2021 · 5 comments · Fixed by #10703
Closed
4 tasks

authz: if grantee doesn't exist, should create account #10590

cmwaters opened this issue Nov 22, 2021 · 5 comments · Fixed by #10703
Assignees

Comments

@cmwaters
Copy link
Contributor

Summary

If the grantee of an authorization doesn't exist, create a new account.

Problem Definition

I was playing with the new authz module. Specifically, I wanted to create a new account and give it access to vote on behalf of my main account. The authorization worked fine but when I tried to send a vote message with my new account, I couldn't because the account didn't exist. Instead I got:

Account does not exist on chain. Send some tokens there before trying to query sequence.

(this is a cosmjs error but it just means it tried to query auth and no account was returned)

Proposal

I'm not too sure what the current policy is on account creation (i.e. should all accounts need a non-zero token balance). If it's possible, I would propose that the authz module also checks if the grantee exists and if not creates the account. I guess there could be a problem with getting the new accounts PubKey. I would just prefer to have to not have to add some tokens to an account first.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@cmwaters
Copy link
Contributor Author

Ah it seems like we do a similar thing with feegrant

// create the account if it is not in account state
granteeAcc := k.authKeeper.GetAccount(ctx, grantee)
if granteeAcc == nil {
granteeAcc = k.authKeeper.NewAccountWithAddress(ctx, grantee)
    k.authKeeper.SetAccount(ctx, granteeAcc)
}

@alexanderbez
Copy link
Contributor

alexanderbez commented Nov 22, 2021

Creating an account is fine IMO (assuming the granter is paying the fees). Also, an account doesn't need a non-zero balance to exist 👍

@amaury1093
Copy link
Contributor

Sounds fine to me too.

I guess there could be a problem with getting the new accounts PubKey.

Not really a problem, just that in TXs that the grantee sends, they need to put a non-empty pub_key in this field. It's an optional field for accounts that already exist in state with an associated pubkey. But if we're creating an acct like in #10590 (comment), then the account's pubkey in state is nil.

@aaronc
Copy link
Member

aaronc commented Dec 8, 2021

I think the workflow here would be to create a feegrant first which creates the account, then to create authorizations

@amaury1093
Copy link
Contributor

amaury1093 commented Dec 9, 2021

From a client UX, my preference would still be that the SDK allows the steps (1) create authz and (2) create feegrant/send tokens to grantee, to be done in whichever order.

The downside i see is that this solution is API breaking (dep from x/authz on x/auth)

@mergify mergify bot closed this as completed in #10703 Dec 10, 2021
mergify bot pushed a commit that referenced this issue Dec 10, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description
If the grantee of an authorization doesn't exist, create a new account.

Closes: #10590 

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this issue May 22, 2023
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description
If the grantee of an authorization doesn't exist, create a new account.

Closes: cosmos#10590 

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
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 a pull request may close this issue.

5 participants