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

transfer-hook-interface: Remove solana-program dependency #7442

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

joncinque
Copy link
Contributor

Problem

The transfer-hook-interface pulls in solana-program, but it doesn't need to.

Summary of changes

Use the component crates. The only breaking change is the re-exports. It still uses a dev-dependency on solana-program since the system program id isn't available yet outside of solana-program.

@joncinque joncinque requested a review from buffalojoec November 1, 2024 12:10
@joncinque
Copy link
Contributor Author

joncinque commented Nov 1, 2024

Oof, never mind, the CI failure here is very legit.

The token-2022 program tests rely on using the "native builtin" hack, where solana-program-test turns a provided function pointer into a program processor. This relies on the syscall stubs, which are not used in the new solana_cpi crate.

To properly test all the transfer hook behavior, we'll need to turn all of the test processors into built BPF programs.

Edit: moving to draft for now

@joncinque joncinque marked this pull request as draft November 1, 2024 12:50
@buffalojoec
Copy link
Contributor

To properly test all the transfer hook behavior, we'll need to turn all of the test processors into built BPF programs.

What's the plan for this, create a "test programs" directory? Could get a little messy.

@joncinque
Copy link
Contributor Author

Yeah that's what I've been doing. A directory of test programs, with built programs in tests/fixtures. I was thinking that we'll need to do this no matter what, if we ever want to move away from solana-program-test, so it's probably worth biting the bullet. It comes out to 4 extra test programs. Other than the extra directories and workspace entries, do you have any other concerns?

@buffalojoec
Copy link
Contributor

Other than the extra directories and workspace entries, do you have any other concerns?

No, I don't think so, it was mainly the workspace members and determining how often we need to build new fixtures, which shouldn't be often.

@joncinque joncinque marked this pull request as ready for review November 1, 2024 14:57
@joncinque
Copy link
Contributor Author

All right, it's all good! Let me know how you like the structure

buffalojoec
buffalojoec previously approved these changes Nov 1, 2024
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 work! This came out cleaner than I expected!

#### Problem

The transfer-hook-interface pulls in solana-program, but it doesn't need
to.

#### Summary of changes

Use the component crates. The only breaking change is the re-exports. It
still uses a dev-dependency on solana-program since the system program
id isn't available yet outside of solana-program.
@mergify mergify bot dismissed buffalojoec’s stale review November 1, 2024 15:51

Pull request has been modified.

@joncinque joncinque merged commit e07fe68 into solana-labs:master Nov 1, 2024
31 checks passed
@joncinque joncinque deleted the nosphook branch November 1, 2024 16:38
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 this pull request may close these issues.

2 participants