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

Add Rust programs using pinocchio #7

Merged
merged 11 commits into from
Oct 31, 2024
Merged

Conversation

febo
Copy link
Contributor

@febo febo commented Oct 10, 2024

This PR adds Rust programs for Transfer-Lamports and CPI using pinocchio. It does not include a Helloworld since it would be the same as the current Rust version.

Results

  • Transfer-Lamports: moves 5 lamports from a source account to a destination
Language CU Usage
Rust 464
Zig 43
C 103
Assembly 22
Rust (pinocchio) 23

This one starts to get interesting since it requires parsing the instruction
input. Since the assembly version knows exactly where to find everything, it can
be hyper-optimized. The C version is also very performant.

Zig's version should perform the same as C, but there are some inefficiencies that
are currently being fixed.

  • CPI: allocates a PDA given by the seed "You pass butter" and a bump seed in
    the instruction data. This requires a call to create_program_address to check
    the address and invoke_signed to CPI to the system program.
Language CU Usage
Rust 3662
Zig 2825
C 3122
Rust (pinocchio) 2816

Copy link
Owner

@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 really good! Once we figure out how to run these, could you add a step in CI to run these? It would look just like rust-test at https://github.com/joncinque/solana-program-rosetta/blob/89efc3b4ebc11139a73a4d5c97f99ab710e29d3c/.github/workflows/main.yml#L46C3-L77C54

But instead of cargo test-sbf it would use test-pinocchio.sh

cpi/pinocchio/src/lib.rs Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way to avoid this copy and reuse the existing tests at cpi/tests? We might be able to get away with something similar to the C / Zig / Assembly test runner, where there's a test-pinocchio.sh script that builds the program and then specifies some environment variable for the program name, ie:

#!/usr/bin/env bash
PROGRAM_NAME="$1"
ROOT_DIR="$(cd "$(dirname "$0")"; pwd)"
set -e
PROGRAM_DIR=$ROOT_DIR/$PROGRAM_NAME
cd $PROGRAM_DIR/pinocchio
cargo build-sbf
ROSETTA_PROGRAM_NAME="pinocchio_rosetta_cpi" SBF_OUT_DIR="target/deploy" cargo test --manifest-path "$PROGRAM_DIR/Cargo.toml"

And then use std::env::var("ROSETTA_PROGRAM_NAME").unwrap_or("solana_program_rosetta_cpi") for the program name in the test.

What do you think?

Copy link
Contributor Author

@febo febo Oct 26, 2024

Choose a reason for hiding this comment

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

Good idea! I had to use a ROSETTA_LIBRARY variable to then choose the program name, since the name has to be a &'static str.

Copy link
Owner

Choose a reason for hiding this comment

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

Same here for the tests -- maybe we can ditch the copy and do something else

@febo febo marked this pull request as draft October 21, 2024 09:11
@febo febo marked this pull request as ready for review October 26, 2024 16:12
@febo febo requested a review from joncinque October 26, 2024 16:12
@leafaar
Copy link

leafaar commented Oct 30, 2024

awesome!

Copy link
Owner

@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.

Thanks for updating these! It's very close, just a few last bits to sort out. And if I'm being too annoying, just let me know and I'll take over 😅

cpi/pinocchio/Cargo.toml Outdated Show resolved Hide resolved
cpi/pinocchio/Cargo.toml Outdated Show resolved Hide resolved
#[tokio::test]
async fn test_cross_program_invocation() {
let program_id = Pubkey::from_str("invoker111111111111111111111111111111111111").unwrap();
let (allocated_pubkey, bump_seed) =
Pubkey::find_program_address(&[b"You pass butter"], &program_id);

let library = std::env::var("ROSETTA_LIBRARY").unwrap_or(String::from("solana_program"));
Copy link
Owner

Choose a reason for hiding this comment

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

Needing to specify the ROSETTA_LIBRARY env variable and also hard-code it into the test seems a bit overkill -- how about having an environment variable like PROGRAM_NAME and just using that or the default?

Then test-pinocchio.sh just needs to combine pinocchio with the program name. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, but we still have the issue that ProgramTest::new expects the program_name as a &'static str. That is the only reason for hard-coding the name on the test file.

Copy link
Owner

Choose a reason for hiding this comment

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

how about using env! in that case? https://doc.rust-lang.org/std/macro.env.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea - ended up using option_env! so we can specify a default value if the variable is not set.

transfer-lamports/pinocchio/Cargo.toml Outdated Show resolved Hide resolved
transfer-lamports/pinocchio/Cargo.toml Outdated Show resolved Hide resolved
transfer-lamports/tests/functional.rs Outdated Show resolved Hide resolved
Copy link
Owner

@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.

Everything looks good, just need to figure out the environment variable for the test

@febo febo requested a review from joncinque October 31, 2024 22:08
Copy link
Owner

@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.

Beautiful, thanks for the contribution!

@joncinque joncinque enabled auto-merge (squash) October 31, 2024 22:36
@joncinque joncinque merged commit 45d9595 into joncinque:main Oct 31, 2024
13 checks passed
@febo febo deleted the pinocchio branch October 31, 2024 22:47
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.

3 participants