Add const pubkey support and make declare_id a declarative macro #2348

kevinheavey · 2024-07-30T13:55:25Z

Problem

Proc macros are slow to compile
Proc macros are harder to move around when they need to import other crates (since Rust doesn't have macro-dependencies yet). In particular this matters for making types available without solana_program.
const base58 decoding can be done with the five8_const crate, which has more useful error messages than the bs58 crate (that repo has a PR to improve this but it's been open for months)

Summary of Changes

Add a from_str_const associated function to Pubkey. I just lifted this from package-metadata: Add macro to define program id from Cargo.toml #1806 which is a draft PR at the time of writing
Replace the declare_id alias in solana_program with a declarative macro of the same name that does the same thing in solana_pubkey (re-exported in solana_program)
Deprecate the program_declare_id! macro in solana_sdk_macro
Convert the pubkey! macro to a declarative macro and deprecate solana_sdk_macro::program_pubkey

juchiast · 2024-10-05T09:42:56Z

sdk/pubkey/src/lib.rs

@@ -304,6 +304,12 @@ impl Pubkey {
        Self(pubkey_array)
    }

+    /// Decode a string into a Pubkey, usable in a const context
+    pub const fn from_str_const(s: &str) -> Self {


Hello, I'm just passing by, I think this could use a &'static str input and renamed to from_static, similar pattern as HeaderValue::from_static. Using &'static will prevent people from accidentally using this function for faillible inputs.

I don't think this will accomplish what you think it will

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0d6c57cbc59f7fafb6dbb22e01c720af

I think what you're really looking for is the thing a macro does which is to check that the input is a (string) literal

something like this, which was the previous solution

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4b9aed32eef1d5ada53b05fd834c0a25

I know it's not a perfect solution, but the function isn't intented to be called with a live 'a, so there is no reason to allow it in the function signature. Using &' static prevent common misuse, if someone leaks a string, they should already know what they are doing. I think, as a library design principle, function signature should clearly communicate what is it use case, using &str tell users that it works with a live 'a.

In the meantime I've undone the deprecation of the pubkey macro and converted it to a declarative macro in solana-pubkey, since this discussion revealed that Pubkey::from_str_const does not make it completely obsolete

febo · 2024-10-11T11:47:52Z

sdk/src/lib.rs

@@ -144,6 +144,10 @@ pub use solana_sdk_macro::declare_deprecated_id;
 /// assert_eq!(id(), my_id);
 /// ```
 pub use solana_sdk_macro::declare_id;
+#[deprecated(
+    since = "2.1.0",
+    note = "Use `solana_program::pubkey::Pubkey::from_str_const` instead"


Here it should say "Use solana_pubkey::pubkey ... ", right?

Good spot, I've also updated it to re-export solana_pubkey::pubkey instead of the one from solana_sdk_macro

This reverts commit 60dbddd.

…ad of the old one

joncinque

Looks great! Thanks for your patience, I wanted to do due diligence and read over the five8_const code, which also required reading firedancer code. Everything is good to go on my side

joncinque · 2024-10-14T12:59:54Z

sdk/macro/src/lib.rs

@@ -204,12 +206,17 @@ pub fn declare_deprecated_id(input: TokenStream) -> TokenStream {
    TokenStream::from(quote! {#id})
 }

+#[deprecated(since = "2.1.0", note = "Use `solana_pubkey::declare_id` instead")]
 #[proc_macro]
 pub fn program_declare_id(input: TokenStream) -> TokenStream {


I'll be honest, until this PR, I hadn't realized there were different versions of these macros for solana_sdk and solana_program. This PR is good as is, but we should definitely consider removing a lot of the extra cruft in favor of solana_pubkey's macros in future work.

cavemanloverboy · 2024-10-14T13:08:25Z

good work

…a-xyz#2348) * make declare_id a declarative macro * remove old comment * deprecate program_declare_id * deprecate pubkey macro * put deprecation on the re-export of the pubkey macro * replace pubkey! with from_str_const in this repo * fmt * remove unused import * Revert "remove unused rustc_version dep from wen-restart (wrong branch)" This reverts commit 60dbddd. * avoid wen-restart changes again * fmt * fix deprecation text * make declare_deprecated_id a declarative macro * put back the deprecation on the re-export of the pubkey macro * fmt * don't deprecate the pubkey macro, but make it a declarative macro * update deprecation note * re-export the new pubkey macro in solana-sdk (with deprecation) instead of the old one

mergify bot added the need:merge-assist label Jul 30, 2024

kevinheavey requested a review from joncinque July 30, 2024 16:20

kevinheavey force-pushed the new-declare-id branch from 0553bfc to 199760f Compare August 7, 2024 16:18

kevinheavey force-pushed the new-declare-id branch 2 times, most recently from e799f9b to 4461538 Compare September 10, 2024 18:38

kevinheavey force-pushed the new-declare-id branch 2 times, most recently from 946d41c to f0ec89b Compare September 25, 2024 11:42

kevinheavey mentioned this pull request Sep 27, 2024

Extract system-instruction crate #3007

Closed

juchiast reviewed Oct 5, 2024

View reviewed changes

kevinheavey force-pushed the new-declare-id branch 3 times, most recently from 893b85a to c129050 Compare October 11, 2024 08:28

kevinheavey mentioned this pull request Oct 11, 2024

remove solana-program from solana-feature-set #3140

Merged

febo reviewed Oct 11, 2024

View reviewed changes

kevinheavey force-pushed the new-declare-id branch 2 times, most recently from 87c3252 to 296f002 Compare October 11, 2024 20:17

kevinheavey added 14 commits October 12, 2024 16:41

make declare_id a declarative macro

bf63890

remove old comment

d5543b1

deprecate program_declare_id

6af60e8

deprecate pubkey macro

28fafb9

put deprecation on the re-export of the pubkey macro

9b22aa2

replace pubkey! with from_str_const in this repo

de462d2

fmt

2d040a4

remove unused import

8553099

Revert "remove unused rustc_version dep from wen-restart (wrong branch)"

a0171bf

This reverts commit 60dbddd.

avoid wen-restart changes again

5ec4ecb

fmt

67f2da4

fix deprecation text

6c89e9e

make declare_deprecated_id a declarative macro

31b2986

put back the deprecation on the re-export of the pubkey macro

e9fae36

kevinheavey added 4 commits October 12, 2024 16:41

fmt

44ec8a8

don't deprecate the pubkey macro, but make it a declarative macro

61794be

update deprecation note

2506d47

re-export the new pubkey macro in solana-sdk (with deprecation) inste…

bfa2bf9

…ad of the old one

kevinheavey force-pushed the new-declare-id branch from 296f002 to bfa2bf9 Compare October 12, 2024 12:41

joncinque approved these changes Oct 14, 2024

View reviewed changes

joncinque merged commit 2ea7fd7 into anza-xyz:master Oct 14, 2024
50 checks passed

kevinheavey deleted the new-declare-id branch October 14, 2024 13:13

joncinque mentioned this pull request Oct 14, 2024

package-metadata: Add macro to define program id from Cargo.toml #1806

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add const pubkey support and make declare_id a declarative macro #2348

Add const pubkey support and make declare_id a declarative macro #2348

kevinheavey commented Jul 30, 2024 •

edited

Loading

juchiast Oct 5, 2024

cavemanloverboy Oct 5, 2024

cavemanloverboy Oct 5, 2024

cavemanloverboy Oct 5, 2024

juchiast Oct 6, 2024

kevinheavey Oct 6, 2024

febo Oct 11, 2024

kevinheavey Oct 11, 2024

joncinque left a comment

joncinque Oct 14, 2024

cavemanloverboy commented Oct 14, 2024

Add const pubkey support and make declare_id a declarative macro #2348

Add const pubkey support and make declare_id a declarative macro #2348

Conversation

kevinheavey commented Jul 30, 2024 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joncinque left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cavemanloverboy commented Oct 14, 2024

kevinheavey commented Jul 30, 2024 •

edited

Loading