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.

Sign up for GitHub

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

Jump to bottom

token group: init interface #5512

Merged
merged 7 commits into from
Oct 18, 2023
Merged

token group: init interface #5512

merged 7 commits into from
Oct 18, 2023

Conversation

buffalojoec
Copy link
Contributor

This PR initializes a newer, slimmed down version of the SPL Token Group interface.

Previous PR: #5190

@buffalojoec buffalojoec mentioned this pull request Oct 11, 2023
Closed
joncinque
joncinque reviewed Oct 13, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

This is looking really good! Just some bits to iron out on the interface, then this should be good to start on

token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
token-group/interface/src/instruction.rs Show resolved Hide resolved
token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
@buffalojoec
Copy link
Contributor Author

OK I've resolved most of the comments, but there's still a few items to discuss:

joncinque
joncinque reviewed Oct 13, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a few last things, then this should be good to go!

Edit: oh, and we'll also need the member mint authority to sign the initialize_member instruction

token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
token-group/interface/src/state.rs Outdated Show resolved Hide resolved
token-group/interface/src/instruction.rs Outdated Show resolved Hide resolved
@buffalojoec
Copy link
Contributor Author

Edit: oh, and we'll also need the member mint authority to sign the initialize_member instruction

Would you say mint authority or update authority? A case could be made for either imo.

@buffalojoec
Copy link
Contributor Author

buffalojoec commented Oct 16, 2023 •
edited
Loading

Also, I think we're good on discriminators:

EDIT: Sorry, you said 100s, updated.

  • InitializeGroup: [121, 113, 108, 39, 54, 51, 0, 4]
  • UpdateGroupAuthority: [161, 105, 88, 1, 237, 221, 216, 203]
  • UpdateGroupMaxSize: [108, 37, 171, 143, 248, 30, 18, 110]
  • InitializeMember: [152, 32, 222, 176, 223, 237, 116, 134]

@buffalojoec Graphite App
Copy link
Contributor Author

buffalojoec commented Oct 16, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

This was referenced Oct 16, 2023
Merged
Closed
@joncinque
Copy link
Contributor

Would you say mint authority or update authority? A case could be made for either imo.

There is no update authority here since we're just initializing the member, right? I think that means we have to go with member mint authority

joncinque
joncinque previously approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks good!

@mergify mergify bot dismissed joncinque’s stale review October 17, 2023 19:55

Pull request has been modified.

@buffalojoec
Copy link
Contributor Author

Bumped that version. Good to go.

@buffalojoec buffalojoec force-pushed the token-group-interface-1 branch 2 times, most recently from 701b548 to 1315e99 Compare October 18, 2023 11:56
joncinque
joncinque previously approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks great!

@mergify mergify bot dismissed joncinque’s stale review October 18, 2023 12:17

Pull request has been modified.

@buffalojoec
Copy link
Contributor Author

Sorry @joncinque! Had to fix a rustfmt error!

@buffalojoec buffalojoec mentioned this pull request Oct 18, 2023
Closed
joncinque
joncinque approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

No worries! Feel free to merge without an explicit re-approval if you just fixed a rustfmt issue

This was referenced Oct 18, 2023
Closed
Closed
Merged
@buffalojoec buffalojoec merged commit a54425b into master Oct 18, 2023
6 checks passed
@buffalojoec buffalojoec deleted the token-group-interface-1 branch October 18, 2023 16:32
@buffalojoec buffalojoec mentioned this pull request Oct 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joncinque joncinque joncinque approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@buffalojoec @joncinque