Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

Add ImmutableOwner extension to block ATA owner authority changes #2854

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

mvines
Copy link
Contributor

@mvines mvines commented Feb 1, 2022

ATA will now set the ImmutableOwner extension on new accounts it creates.

Fixes #2640

@@ -77,7 +81,7 @@ fn get_extension_indices<V: Extension>(
let v_account_type = V::TYPE.get_account_type();
while start_index < tlv_data.len() {
let tlv_indices = get_tlv_indices(start_index);
if tlv_data.len() <= tlv_indices.value_start {
if tlv_data.len() < tlv_indices.value_start {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙋🏼 <= to < was needed to support extensions that hold no data, like the FixedOwner extension

@mvines
Copy link
Contributor Author

mvines commented Feb 1, 2022

I'm not sure I love the name "FixedOwner". Drive-by bikeshedding welcome!

@t-nelson
Copy link
Contributor

t-nelson commented Feb 1, 2022

Drive-by bikeshedding welcome!

ImmutableOwner ?

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Functionality looks good. Test failures look legit, though (related to rent-exempt minimum).

I do think we should come up with a different name, since in addition to "Static", "Fixed" can also mean "Repaired" (and even though that's slightly true in this case, it's also confusing).
I like ImmutableOwner reasonably well; other options: FrozenOwner, LockedOwner (both of which suffer from implying that the operation can be un-done), StaticOwner

@mvines
Copy link
Contributor Author

mvines commented Feb 1, 2022

I like ImmutableOwner reasonably well; other options: FrozenOwner, LockedOwner (both of which suffer from implying that the operation can be un-done), StaticOwner

ImmutableOwner ain't bad. @joncinque - what say you? 👍🏼 / 👎🏼

@joncinque
Copy link
Contributor

ImmutableOwner is my favorite of the bunch

@mvines mvines changed the title Add FixedOwner extension to block ATA owner authority changes Add ImmutableOwner extension to block ATA owner authority changes Feb 1, 2022
CriesofCarrots
CriesofCarrots previously approved these changes Feb 1, 2022
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

lgtm if CI is happy!

@@ -64,6 +67,7 @@ fn get_tlv_indices(type_start: usize) -> TlvIndices {

/// Helper struct for returning the indices of the type, length, and value in
/// a TLV entry
#[derive(Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, I added and removed this about a dozen times when working on #2824

@mergify mergify bot dismissed CriesofCarrots’s stale review February 1, 2022 20:32

Pull request has been modified.

@mvines mvines merged commit 115c3c4 into solana-labs:master Feb 1, 2022
@mvines mvines deleted the fo branch February 1, 2022 20:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Associated token account owner is changeable
4 participants