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

fix: 'owner' should be 'AccountOwner' in associated-token-account.md #6126

Closed
wants to merge 2 commits into from

Conversation

mikemaccana
Copy link
Contributor

@mikemaccana mikemaccana commented Jan 12, 2024

The old wording seemed incorrect. Solana uses the term 'owner' to mean program owner. I believe this sentence is saying the AccountOwner on the new ATA will be the associated wallet, can someone confirm that's correct?

The old wording seemed incorrect. Solana uses the term 'owner' to mean program owner. I believe this sentence is saying the authority on the new ATA will be the associated wallet, can someone confirm that's correct?
@mergify mergify bot added the community Community contribution label Jan 12, 2024
@mikemaccana mikemaccana changed the title fix: 'owner' should be 'authority' in associated-token-account.md fix: 'owner' should be 'AccountOwner' in associated-token-account.md Jan 12, 2024
@buffalojoec
Copy link
Contributor

buffalojoec commented Jan 12, 2024

So, technically the field that represents the controlling authority of a Token Account is called owner.

However, the SetAuthority command used to change it is called AccountOwner.

match authority_type {
AuthorityType::AccountOwner => {
Self::validate_owner(
program_id,
&account.owner,
authority_info,
account_info_iter.as_slice(),
)?;
if let COption::Some(authority) = new_authority {
account.owner = authority;
} else {
return Err(TokenError::InvalidInstruction.into());
}
account.delegate = COption::None;
account.delegated_amount = 0;
if account.is_native() {
account.close_authority = COption::None;
}
}

As you can see in that snippet, it will update owner based on that set_authority call.

So, although I think it's a bit confusing, owner is actually probably more accurate here.

Maybe we can reword this a bit and say:

Regardless of the creator, the new associated token account's
[`owner`](https://github.com/solana-labs/solana-program-library/blob/23916b24700cd222ad2397e9de8162ad6980e2e8/token/program/src/state.rs#L92-L93)
field will be the wallet, thus it will be controlled by the wallet, as
if the wallet itself had created the associated token account.

micro-nit: can you make sure to wrap the columns at 80?

@mikemaccana
Copy link
Contributor Author

mikemaccana commented Jan 12, 2024

Got it! Your suggestion is definitely clearer. It sounds like the real issue is the code itself has two things called owner - account owner and program owner. It might be even better to switch to more distinct terms (rather than reuse 'owner') in order to avoid confusion. I know that sounds like significant change but if there's going to be ten times as many Solana developers in the future, it may be a good one to make sooner rather than later.

micro-nit: can you make sure to wrap the columns at 80?

Sure! Existing code is a mix of hard and soft wrapped and I didn't want to reformat unrelated code.

@buffalojoec
Copy link
Contributor

Got it! Your suggestion is definitely clearer. It sounds like the real issue is the code itself has two things called owner - account owner and program owner. It might be even better to switch to more distinct terms (rather than reuse 'owner') in order to avoid confusion. I know that sounds like significant change but if there's going to be ten times as many Solana developers in the future, it may be a good one to make sooner rather than later.

Agreed, and to make matters worse we also call it authority in a lot of places, too. 💀

@github-actions github-actions bot added the stale [bot only] Added to stale content; will be closed soon label Jan 30, 2024
@github-actions github-actions bot closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution stale [bot only] Added to stale content; will be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants