-
Notifications
You must be signed in to change notification settings - Fork 7
geolocation: add GeoProbe state and CRUD instructions #3120
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
|
|
||
| ### Security | ||
|
|
||
| 1. **PDA ownership verification**: Always verify the owner of PDA accounts (both internal PDAs and those from other programs like serviceability) to prevent being tricked into reading an account owned by another program. For serviceability accounts, verify the owner is the serviceability program ID. For your own PDAs, verify the owner is `program_id`. | ||
| 1. **PDA ownership verification**: Always verify the owner of PDA accounts (both internal PDAs and those from other programs like serviceability) to prevent being tricked into reading an account owned by another program. For serviceability accounts, verify the owner is the serviceability program ID. For your own PDAs, verify the owner is `program_id`. Exception: for singleton PDAs (e.g., ProgramConfig, GlobalState) the account type discriminator check via `try_from` is sufficient — the ownership check is harmless but redundant since there is only one valid account. | ||
|
|
||
| 2. **System program validation**: Checks for the system program are unnecessary because the system interface builds instructions using the system program as the program ID. If the wrong program is provided, you'll get a revert automatically. | ||
|
|
||
|
|
@@ -28,6 +28,18 @@ | |
|
|
||
| 2. **Use BorshDeserializeIncremental**: For instruction arguments that may gain new optional fields over time, use `BorshDeserializeIncremental` or derive `BorshDeserialize`. | ||
|
|
||
| ### Testing | ||
|
|
||
| 1. **Assert specific errors**: Tests should assert specific error types (e.g., `ProgramError::Custom(17)`), not just `.is_err()`. This catches regressions where the instruction fails for the wrong reason. | ||
|
|
||
| 2. **Don't test framework functionality**: Avoid writing tests that only exercise SDK/framework behavior (e.g., testing that `Pubkey::find_program_address` is deterministic or produces different outputs for different inputs). Focus tests on your program's logic. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess when the unit tests were generated in this PR, claude forgot about this |
||
|
|
||
| 3. **Integration tests for all processors**: Every processor function (instruction handler) should have corresponding integration tests in the `tests/` directory. These tests should cover: | ||
| - Success cases with valid inputs | ||
| - All error cases (invalid inputs, unauthorized signers, wrong account states) | ||
| - Edge cases (boundary values, empty collections, maximum sizes) | ||
| - State transitions (account creation, updates, deletion) | ||
|
|
||
| ### Program Upgrades | ||
|
|
||
| 1. **Use standard interfaces**: Use `solana-loader-v3-interface` to parse `UpgradeableLoaderState` rather than implementing your own parser. The interface crate provides well-tested, maintained implementations. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,68 @@ | ||
| use solana_program::pubkey::Pubkey; | ||
|
|
||
| use crate::seeds::{SEED_PREFIX, SEED_PROGRAM_CONFIG}; | ||
| use crate::seeds::{SEED_PREFIX, SEED_PROBE, SEED_PROGRAM_CONFIG}; | ||
|
|
||
| pub fn get_program_config_pda(program_id: &Pubkey) -> (Pubkey, u8) { | ||
| Pubkey::find_program_address(&[SEED_PREFIX, SEED_PROGRAM_CONFIG], program_id) | ||
| } | ||
|
|
||
| pub fn get_geo_probe_pda(program_id: &Pubkey, code: &str) -> (Pubkey, u8) { | ||
| Pubkey::find_program_address(&[SEED_PREFIX, SEED_PROBE, code.as_bytes()], program_id) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| fn test_program_id() -> Pubkey { | ||
| Pubkey::new_unique() | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_program_config_pda_is_deterministic() { | ||
| let program_id = test_program_id(); | ||
| let (pda1, bump1) = get_program_config_pda(&program_id); | ||
| let (pda2, bump2) = get_program_config_pda(&program_id); | ||
| assert_eq!(pda1, pda2); | ||
| assert_eq!(bump1, bump2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_program_config_pda_differs_by_program_id() { | ||
| let (pda1, _) = get_program_config_pda(&Pubkey::new_unique()); | ||
| let (pda2, _) = get_program_config_pda(&Pubkey::new_unique()); | ||
| assert_ne!(pda1, pda2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_geo_probe_pda_is_deterministic() { | ||
| let program_id = test_program_id(); | ||
| let (pda1, bump1) = get_geo_probe_pda(&program_id, "probe-a"); | ||
| let (pda2, bump2) = get_geo_probe_pda(&program_id, "probe-a"); | ||
| assert_eq!(pda1, pda2); | ||
| assert_eq!(bump1, bump2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_geo_probe_pda_differs_by_code() { | ||
| let program_id = test_program_id(); | ||
| let (pda1, _) = get_geo_probe_pda(&program_id, "probe-a"); | ||
| let (pda2, _) = get_geo_probe_pda(&program_id, "probe-b"); | ||
| assert_ne!(pda1, pda2); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_geo_probe_pda_differs_by_program_id() { | ||
| let (pda1, _) = get_geo_probe_pda(&Pubkey::new_unique(), "probe-a"); | ||
| let (pda2, _) = get_geo_probe_pda(&Pubkey::new_unique(), "probe-a"); | ||
| assert_ne!(pda1, pda2); | ||
| } | ||
|
Comment on lines
+21
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests don't seem necessary (and would argue the one below this comment isn't either, too). What do you think? |
||
|
|
||
| #[test] | ||
| fn test_different_pda_types_do_not_collide() { | ||
| let program_id = test_program_id(); | ||
| let (config_pda, _) = get_program_config_pda(&program_id); | ||
| let (probe_pda, _) = get_geo_probe_pda(&program_id, "code"); | ||
| assert_ne!(config_pda, probe_pda); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this