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

tests: Add process instruction checks #19

Merged
merged 4 commits into from
Jan 16, 2025
Merged

tests: Add process instruction checks #19

merged 4 commits into from
Jan 16, 2025

Conversation

febo
Copy link
Contributor

@febo febo commented Jan 12, 2025 •
edited
Loading

Problem

While PR #18 updates the processor tests to use mollusk, the validation checks are done "outside" mollusk.

Solution

This PR adds Checks to process instruction. It leaves the existing validation checks as well to guarantee the same behaviour.

@febo febo requested review from joncinque and buffalojoec January 12, 2025 15:26
@febo febo force-pushed the febo/mollusk-checks branch from a27dd01 to 4c45f35 Compare January 14, 2025 15:35
@joncinque joncinque mentioned this pull request Jan 15, 2025
Merged
joncinque
joncinque reviewed Jan 15, 2025
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 overall! Just one tiny thing and the set_expected_data part which should get resolved here or in #18

program/tests/assert_instruction_count.rs
@@ -5,7 +5,7 @@ mod setup;
use {
mollusk_svm::{result::Check, Mollusk},
solana_sdk::{
account::{AccountSharedData, ReadableAccount},
account::{Account as SolanaAccount, ReadableAccount},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change required by the new version of mollusk?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we switched to Account in the API to avoid any footguns for the shared reference over the data buffer. We found some shallow clone bugs.

Luckily the APIs for Account and AccountSharedData are very similar. Do you think it's safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely safe, since it makes it clear that mollusk creates copies of all the resulting accounts rather than modifying them in place. If performance ever becomes an issue, then it might be worth avoiding the account clones.

program/tests/processor.rs Outdated Show resolved Hide resolved
buffalojoec
buffalojoec reviewed Jan 15, 2025
View reviewed changes
program/tests/close_account.rs Show resolved Hide resolved
program/Cargo.toml
@@ -22,7 +22,7 @@ thiserror = "2.0"

[dev-dependencies]
lazy_static = "1.5.0"
mollusk-svm = "0.0.13"
mollusk-svm = { version = "0.0.13", git = "https://github.com/buffalojoec/mollusk.git" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I can publish today, if you want to move this off a Git dependency before merging.

@febo febo force-pushed the febo/mollusk-processor-tests branch from 9b284cb to b8d47b7 Compare January 15, 2025 10:33
@febo febo force-pushed the febo/mollusk-checks branch 2 times, most recently from 21ec242 to 3194d59 Compare January 15, 2025 13:17
@febo
Copy link
Contributor Author

febo commented Jan 15, 2025

Looks great overall! Just one tiny thing and the set_expected_data part which should get resolved here or in #18

@joncinque Added the set_expected_data checks in this PR.

@febo febo requested review from joncinque and buffalojoec January 15, 2025 13:27
joncinque
joncinque approved these changes Jan 15, 2025
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 to me!

Base automatically changed from febo/mollusk-processor-tests to main January 16, 2025 12:46
@febo febo force-pushed the febo/mollusk-checks branch from 3194d59 to e47a0e1 Compare January 16, 2025 12:57
@febo febo merged commit 8fe3789 into main Jan 16, 2025
12 checks passed
@febo febo deleted the febo/mollusk-checks branch January 16, 2025 13:28
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

@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.
3 participants
@febo @joncinque @buffalojoec