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 22: Add functionality to update extra_account_metas after initializing. #5894

Merged
merged 40 commits into from
Dec 7, 2023
Merged

Token 22: Add functionality to update extra_account_metas after initializing. #5894

merged 40 commits into from
Dec 7, 2023

Conversation

tonton-sol
Copy link
Contributor

@tonton-sol tonton-sol commented Nov 26, 2023 •
edited
Loading

This is my first attempt at adding an update functionality, issue #5662. I added an update function for tlv-account-resolution::ExtraAccountMetaList that will realloc the original data then rewrite the account metas. I also added the correct instruction handling to the cli, interface, and program.

The account size is increased if needed by the program. As far as I am aware there is no way to decrease the size of an account, only make it larger.

This is my first open-source contribution, so please let me know if I did something improper dev-ops wise.

Thank you!

@mergify mergify bot added the community Community contribution label Nov 26, 2023
@tonton-sol
Copy link
Contributor Author

Fixed two issues.

buffalojoec
buffalojoec reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Hey thanks for contributing! This is looking really good.

It would be great if you added some tests for the new update function and processor!

token/transfer-hook/interface/src/instruction.rs Outdated Show resolved Hide resolved
token/transfer-hook/interface/src/instruction.rs Outdated Show resolved Hide resolved
token/transfer-hook/interface/src/instruction.rs Outdated Show resolved Hide resolved
token/transfer-hook/example/src/processor.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Show resolved Hide resolved
token/transfer-hook/cli/src/main.rs Outdated Show resolved Hide resolved
@tonton-sol
Copy link
Contributor Author

@buffalojoec I believe I got all of your suggested edits. Please let me know if the tests are appropriate. Thank you!

buffalojoec
buffalojoec reviewed Nov 28, 2023
View reviewed changes
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Thanks for the continued awesome work! I left a few more comments, mostly little stuff, but this is moving along really nicely!

token/transfer-hook/interface/src/instruction.rs Outdated Show resolved Hide resolved
token/transfer-hook/interface/src/instruction.rs Outdated Show resolved Hide resolved
token/transfer-hook/example/src/processor.rs Outdated Show resolved Hide resolved
token/transfer-hook/cli/src/main.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Outdated Show resolved Hide resolved
token/transfer-hook/example/tests/functional.rs Show resolved Hide resolved
token/transfer-hook/example/src/processor.rs Show resolved Hide resolved
token/transfer-hook/cli/src/main.rs Outdated Show resolved Hide resolved
buffalojoec
buffalojoec reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Nice!! Just a few more small items and we should be cruising 💪🏼 💪🏼

token/transfer-hook/cli/src/main.rs Outdated Show resolved Hide resolved
token/transfer-hook/cli/src/main.rs Outdated Show resolved Hide resolved
token/transfer-hook/cli/src/main.rs Outdated Show resolved Hide resolved
token/transfer-hook/cli/src/main.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Outdated Show resolved Hide resolved
token/transfer-hook/interface/src/instruction.rs Outdated Show resolved Hide resolved
@tonton-sol
Copy link
Contributor Author

@tonton-sol just re-request my review or tag me whenever you're ready for another look.

I think its ready 👍

buffalojoec
buffalojoec reviewed Dec 1, 2023
View reviewed changes
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

A few tiny things and we're ready to ship. Massive thanks again!

token/transfer-hook/cli/src/main.rs Outdated Show resolved Hide resolved
token/transfer-hook/example/src/processor.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Outdated Show resolved Hide resolved
token/transfer-hook/example/tests/functional.rs Show resolved Hide resolved
buffalojoec
buffalojoec reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Just have to update the check and we can punch this in 💪🏼

Again, much appreciated continued effort!

token/transfer-hook/example/src/processor.rs Outdated Show resolved Hide resolved
libraries/tlv-account-resolution/src/state.rs Outdated Show resolved Hide resolved
@tonton-sol
Copy link
Contributor Author

tonton-sol commented Dec 7, 2023 •
edited
Loading

@buffalojoec alright fixed! Thank you for your help!

buffalojoec
buffalojoec approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Awesome job!! Nice contribution to SPL 🤙🏼

@buffalojoec buffalojoec merged commit 2b8c48c into solana-labs:master Dec 7, 2023
33 checks passed
This was referenced Dec 9, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buffalojoec buffalojoec buffalojoec approved these changes

Assignees
No one assigned
Labels
community Community contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tonton-sol @buffalojoec