Update solana-sdk-macro to use solana_pubkey::Pubkey#187
Update solana-sdk-macro to use solana_pubkey::Pubkey#187RaghavenderSingh wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
I wonder if there's a better way to do this, since it's going to be a breaking change anyway.
With the current implementation, anyone using solana_sdk would have to also add solana_pubkey to use these macros, which could be a bit annoying, since Pubkey is already offered through solana_sdk.
We could follow an explicit import pattern, where the macro lib defaults to solana_pubkey, but developers can override it to use solana_sdk::pubkey instead. Here are a few examples:
num_derive: https://docs.rs/num-derive/latest/num_derive/#explicit-importspl_program_error:
Of course, if any maintainers think this is too much, and we should just break it, I will concede. 😀
|
@buffalojoec you're absolutely right, thanks for catching that! |
- Replace ::solana_sdk::pubkey::Pubkey with ::solana_pubkey::Pubkey in SdkPubkey, Id, IdDeprecated, and Pubkeys structs - Keep ::solana_program::pubkey::Pubkey for ProgramSdk* structs - Maintain API compatibility with existing declare_id! and pubkeys! usage Fixes anza-xyz#179
RaghavenderSingh
force-pushed
the
fix/update-sdk-macro-to-solana-pubkey
branch
from
56bb03b to
ba6f857
Compare
|
@RaghavenderSingh I'm not sure if you saw my top-level comment here. |
- Add optional solana_pubkey parameter for custom crate paths - Maintain full backward compatibility with existing usage - Follow pattern established by num_derive and spl_program_error - Add comprehensive test coverage for new functionality - Include error handling for invalid parameters Closes #[issue-number-if-exists]
- Add optional solana_pubkey parameter for custom crate paths
- Support both SDK and program contexts with appropriate defaults
- Maintain full backward compatibility with existing usage
- Enhanced parameter parsing with better error handling
- Follow established patterns from num_derive and spl_program_error
This enables flexible Pubkey imports while preserving existing functionality:
- declare_id!("program_id", solana_pubkey = "custom::path")
- declare_id!("program_id") continues to work unchanged
Key improvements:
- parse_solana_pubkey_param() for flexible import handling
- parse_sdk_pubkey_param() for SDK-specific context
- Enhanced SdkPubkey struct with pubkey_type support
- Comprehensive error handling for invalid parameters
Addresses Solana v3 modularization needs and complex workspace configurations.
- Remove unnecessary blank line in parse_solana_pubkey_param function - Comply with project formatting standards
buffalojoec
left a comment
buffalojoec
left a comment
There was a problem hiding this comment.
Thanks for keeping this going! I left some suggestions for compacting the code, and we also need to test this new feature for providing the pubkey import path.
| }, | ||
| }; | ||
|
|
||
| fn parse_solana_pubkey_param(input: ParseStream) -> Result<proc_macro2::TokenStream> { |
There was a problem hiding this comment.
This function and the one below it are identical, with the exception of the returned tokens.
There was a problem hiding this comment.
In fact, I think all of this duplicated code you've got in these new functions can be compacted onto an enum:
enum PubkeyImport {
SolanaPubkey,
SolanaProgram,
SolanaSdk,
}
impl Parse for PubkeyImport { /* ... */ }
impl ToTokens for PubkeyImport { /* ... */ }There was a problem hiding this comment.
Hey @buffalojoec!
Yeah you're totally right, that's way too much duplicated code. I was so focused on getting the feature working that I didn't step back and see the bigger picture. The enum approach is much cleaner - thanks for the suggestion!
I'll refactor this to use the PubkeyImport enum pattern you outlined. Should be pretty straightforward to consolidate all that parsing logic into one place and just have different default behaviors.
Also need to add some tests for the explicit import feature - completely forgot about that part. Will get both of those done.
Thanks for keeping me honest on the code quality! 😅
Replace duplicated parsing functions with a unified PubkeyImport enum pattern: - Add PubkeyImport enum with SolanaPubkey, SolanaProgram, SolanaSdk variants - Implement Parse and ToTokens traits for centralized import handling - Eliminate code duplication in parse_solana_pubkey_param functions - Update all struct implementations to use consolidated logic - Add comprehensive tests for explicit import patterns Addresses review feedback from buffalojoec in PR anza-xyz#187.
Kept our PubkeyImport enum implementation which addresses buffalojoec's review feedback to eliminate code duplication with a cleaner pattern.
Remove dev-dependencies as noted by buffalojoec - the macro only generates tokens and doesn't need actual crate dependencies. Removed explicit import tests since the macro generates its own inline tests for each declare_id\! usage.
Clean up Cargo.lock since dev-dependencies were removed from sdk-macro.
2 participants
Fixes #179