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

Extract pubkey crate #2394

Merged
merged 56 commits into from
Sep 10, 2024
Merged

Extract pubkey crate #2394

merged 56 commits into from
Sep 10, 2024

Conversation

kevinheavey
Copy link

Problem

solana_program::pubkey can get its own crate once #2015 is merged

Summary of Changes

  • move pubkey.rs to its own crate and re-export in solana-program
  • move relevant syscall definitions to the new crate and re-export them in their old location
  • make the code requiring curve25519-dalek and sha2 optional (these are methods so they can't be moved to separate crates)
  • make bytemuck and serde optional
  • make std optional but enable it by default

@kevinheavey kevinheavey force-pushed the extract-pubkey branch 2 times, most recently from db51566 to b08d018 Compare August 9, 2024 12:08
Copy link

mergify bot commented Aug 9, 2024

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@kevinheavey kevinheavey force-pushed the extract-pubkey branch 3 times, most recently from d7f9bc5 to 1bffbcf Compare September 5, 2024 12:27
@kevinheavey kevinheavey marked this pull request as ready for review September 5, 2024 13:03
@kevinheavey kevinheavey force-pushed the extract-pubkey branch 2 times, most recently from ec41503 to cdbc7a4 Compare September 6, 2024 20:21
@kevinheavey
Copy link
Author

For checking the feature powerset I've used cargo hack check --feature-powerset --exclude-features frozen-abi --all-targets and cargo hack check --feature-powerset --exclude-features frozen-abi --all-targets --at-least-one-of 'std,' --target wasm32-unknown-unknown

Copy link

@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 overall! Mostly small things

sdk/hash/src/lib.rs Outdated Show resolved Hide resolved
sdk/program/src/message/legacy.rs Outdated Show resolved Hide resolved
sdk/program/src/message/legacy.rs Outdated Show resolved Hide resolved
sdk/pubkey/Cargo.toml Outdated Show resolved Hide resolved
sdk/pubkey/src/lib.rs Outdated Show resolved Hide resolved
sdk/pubkey/src/lib.rs Show resolved Hide resolved
Comment on lines +183 to +205
impl FromPrimitive for ParsePubkeyError {
#[inline]
fn from_i64(n: i64) -> Option<Self> {
if n == ParsePubkeyError::WrongSize as i64 {
Some(ParsePubkeyError::WrongSize)
} else if n == ParsePubkeyError::Invalid as i64 {
Some(ParsePubkeyError::Invalid)
} else {
None
}
}
#[inline]
fn from_u64(n: u64) -> Option<Self> {
Self::from_i64(n as i64)
}
}

Choose a reason for hiding this comment

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

Same as with the other FromPrimitive implementation, is there a way to add an exhaustiveness check? If not, would you mind adding a simple test to make sure that the conversion works? ie

        macro_rules! ensure_mapping {
            ($ty: path, $($num: literal => $variant: path),+ $(,)?) => {
                // assert that the given values produce the expected variants
                $(assert_eq!(<$ty>::from($num), $variant);)+
                $(assert_eq!(<$ty>::from_i64($num), Some($variant));)+
                $(assert_eq!(<$ty>::from_u64($num), Some($variant));)+

                // this generated fn will never be called but will produce a
                // non-exhaustive pattern error if you've missed a variant
                #[allow(dead_code)]
                fn check_all_covered(err: $ty) {
                    match err {
                        $($variant => {})+
                    };
                }
            }
        }
    #[test]
    fn test_exhaustive_conversion() {
        ensure_mapping! {
            PubkeyError,
            0 => PubkeyError::MaxSeedLengthExceeded,
            1 => PubkeyError::InvalidSeeds,
            2 => PubkeyError::IllegalOwner,
        }
        ensure_mapping! {
            ParsePubkeyError,
            0 => ParsePubkeyError::WrongSize,
            1 => ParsePubkeyError::Invalid,
        }
    }

You'll need to add a From<u64> implementation for ParsePubkeyError to get this to work, unless you have a more clever way to only do that check for the first enum

Copy link
Author

Choose a reason for hiding this comment

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

I used strum to iterate through variants and check that strum's from_repr matches our from_i64:

for variant in PubkeyError::iter() {
    let variant_i64 = variant as i64;
    assert_eq!(
        PubkeyError::from_repr(variant_i64 as usize),
        PubkeyError::from_i64(variant_i64)
    );
}

sdk/pubkey/src/lib.rs Show resolved Hide resolved
sdk/pubkey/src/lib.rs Outdated Show resolved Hide resolved
@@ -639,8 +773,8 @@ impl Pubkey {
crate::syscalls::sol_log_pubkey(self.as_ref() as *const _ as *const u8)
};

#[cfg(not(target_os = "solana"))]
crate::program_stubs::sol_log(&self.to_string());

Choose a reason for hiding this comment

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

Side note for me: this is fine for now, but we should restore these stubs at some point, probably through a new crate which just contains the stubs.

Copy link

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

One last bit, then this is good to go on my side! @yihau , can you accept ownership of solana-pubkey?

sdk/pubkey/src/lib.rs Show resolved Hide resolved
Copy link

@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, thanks! I'll approve once the crate check passes

@yihau
Copy link
Member

yihau commented Sep 10, 2024

crate-check passed!

@kevinheavey kevinheavey added the automerge automerge Merge this Pull Request automatically once CI passes label Sep 10, 2024
Copy link

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

Great work, thanks for the contribution!

@mergify mergify bot merged commit 9e7637a into anza-xyz:master Sep 10, 2024
54 checks passed
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* extract solana-pubkey from solana-program

* make curve25519_dalek an optional dep

* make sha2 optional in solana-pubkey

* move pubkey wasm code to solana-pubkey

* make serde optional in solana-pubkey

* make bytemuck optional in solana-pubkey

* put syscalls behind target_os = "solana"

* move rand to dev deps

* remove thiserror

* remove num_derive

* make std optional

* use std when target_arch = "wasm32"

* fix frozen-abi support

* update digests

* update digests

* update nits.sh

* update lock file

* make some doc examples text-only because order-crates-for-publishing.py is wrong

* add dev-context-only-utils to appease ci

* fmt

* fix unused import when target_os = "solana"

* fix imports in wasm target

* fix import issue

* activate std feat when borsh feat is activated

* fix a conditional import

* fix more feature issues

* add default-features = false (otherwise we can't disable default features anywhere in the workspace)

* activate std feature explicitly

* clean up imports

* fix test features

* fix lints

* post-rebase fixes

* make FromStr, Display and Debug no_std

* fmt

* update digest

* fix duplicate line post-rebase

* stop avoiding circular dev dep since CI now accommodates this

* make rustc_version optional

* fix doc link

* fix frozen-abi deps

* update digests

* fmt

* don't require std for FromStr

* simplify some imports

* use as_ref instead of to_bytes

Co-authored-by: Jon C <[email protected]>

* use as_ref instead of to_bytes

Co-authored-by: Jon C <[email protected]>

* remove unnecessary test criterion in #[cfg()]

Co-authored-by: Jon C <[email protected]>

* remove unnecessary test criterion in #[cfg()]

Co-authored-by: Jon C <[email protected]>

* remove unrelated change

* but don't remove #[cfg(test)]

* call out doc tests in explanation of circular dev dep

* add missing conversion

* remove unnecessary #[cfg(feature = "std")]

* use strum to check that FromPrimitive impls are exhaustive

* sort deps

* add test for From<u64>

---------

Co-authored-by: Jon C <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge automerge Merge this Pull Request automatically once CI passes need:merge-assist
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants