refactor: sablier-common crate - #257
Conversation
2924b24 to
fdf26af
Compare
Co-authored-by: Iaroslav Mazur <iaroslav.mazur@proton.me>
chore: remove lite anchor just command
Replace token metadata program references with MPL Core across all fuzz test files: types.rs (regenerated), transaction files, fuzz_accounts, constants, and helpers. Update justfile to auto-download mpl_core_program.so and chainlink_program.so fixtures. Remove unused merkle-instant types from generated types.rs. Extract get_linear_params() as a shared helper.
Share chainlink_sol_usd_feed_mock.json between Anchor TS tests and Trident fuzz tests. ChainlinkMock now reads from the JSON instead of hardcoding the base64 data.
- Fix missing saturating_sub in get_refundable_amount and get_withdrawable_amount - Fix end <= now divergence from on-chain code (now > end) - Simplify account_exists to only check lamports - Add universal invariant checks (amounts, timestamps, status, token balance conservation) - Call check_universal_invariants at the end of all transaction assertion functions
- Extract withdraw_common.rs with shared setup and assertions for withdraw/withdraw_max - Extract create_common.rs with shared PDA derivation and assertions for create variants - Add setup_view_flow helper in test_fuzz.rs for view instruction flows - Rename assertions() to assert_cancel() in cancel.rs - Rename is_default_stream to use_default_stream
…r helpers - Add Token-2022 alongside SPL Token with random selection per flow - Extract view instructions into standalone transaction files - Move parse_return_data to helpers/getters.rs - Refactor warp_to_timestamp calls into reusable warp_to method - Rewrite invariants with clearer status-specific checks - Branch mint_deposit_tokens on token program type
- Assert funder_ata balance decreases by deposit_amount after create - Assert start_time matches now for create_with_durations - Add check_settled_stream and check_streaming_stream invariants - Assert success on all mint/ATA init transactions - Rename select_random_token to randomize_deposit_token - Simplify is_token_2022 to string comparison
- Add universal invariants: depletion biconditional, withdrawn/withdrawable <= streamed - Add status-specific: settled streamed==deposited, pending streamed/withdrawable/withdrawn==0 - Fix streaming streamed>0 assertion (only valid post-cliff) - Add streaming withdrawable==streamed-withdrawn, canceled withdrawn<deposited-refunded - Rename _pubkey -> _pk, get_ata_token_balance -> get_ata_balance - Use "must" in all assertion messages - Add section comments to view ix files
Add rust-check-trident / rust-write-trident recipes (fmt + clippy) and wire them into full-check / full-write. Add unconditional Trident code checks step to CI. Document fuzz test validation and conventions in CLAUDE.md. One-time cargo fmt pass on trident-tests/.
Add actions/cache for ~/.cargo/bin/trident keyed on version + OS. Gate the install step on cache miss and build cache miss (binary is only needed when tests run). Use --locked for reproducibility. Also fix .gitignore to use the explicit path for fuzz artifacts.
fdf26af to
6e45345
Compare
WalkthroughWorkspace updated to include Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/sablier-common/src/constants.rs (1)
2-2: Consider using an integer literal for clarity.Using
1e9 as u64works correctly but the floating-point literal is slightly less idiomatic for a constant that represents an exact integer value.✨ Suggested improvement
-pub const LAMPORTS_PER_SOL: u64 = 1e9 as u64; // 1 billion lamports in 1 SOL +pub const LAMPORTS_PER_SOL: u64 = 1_000_000_000; // 1 billion lamports in 1 SOLAlternatively, consider re-exporting
solana_program::native_token::LAMPORTS_PER_SOLif the dependency is already available throughanchor-lang.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/sablier-common/src/constants.rs` at line 2, The constant LAMPORTS_PER_SOL is defined using a float cast ("1e9 as u64"); change it to an integer literal for clarity and correctness (e.g., use 1_000_000_000u64) or, if available, re-export the canonical value from solana_program::native_token::LAMPORTS_PER_SOL instead of redefining it; update the definition of LAMPORTS_PER_SOL in constants.rs accordingly.crates/sablier-common/src/fee_collection.rs (1)
19-20: Consider usingsaturating_addinstead ofchecked_add().unwrap().While the overflow is extremely unlikely in practice (rent-exempt minimum + 1M lamports), using
saturating_addwould be more consistent with the defensive approach used on line 23 and avoids any potential panic.🛡️ Suggested fix
const SAFE_RENT_BUFFER_LAMPORTS: u64 = 1_000_000; // 0.001 SOL - let safe_minimum = rent_exempt_minimum.checked_add(SAFE_RENT_BUFFER_LAMPORTS).unwrap(); + let safe_minimum = rent_exempt_minimum.saturating_add(SAFE_RENT_BUFFER_LAMPORTS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/sablier-common/src/fee_collection.rs` around lines 19 - 20, Replace the panic-prone addition when computing safe_minimum by using saturating_add instead of rent_exempt_minimum.checked_add(SAFE_RENT_BUFFER_LAMPORTS).unwrap(); locate the constant SAFE_RENT_BUFFER_LAMPORTS and the variable safe_minimum calculation and change it to use rent_exempt_minimum.saturating_add(SAFE_RENT_BUFFER_LAMPORTS) so the code matches the defensive pattern used elsewhere (e.g., the logic on line 23).crates/sablier-common/Cargo.toml (1)
10-13: Consider pinning the git dependency to a specific commit or tag.The
chainlink_solanadependency references a branch (solana-2.1) without a pinned revision. This can lead to non-reproducible builds if the branch is updated upstream.🔧 Suggested fix: Pin to a specific commit
- chainlink_solana = { git = "https://github.com/smartcontractkit/chainlink-solana", branch = "solana-2.1" } + chainlink_solana = { git = "https://github.com/smartcontractkit/chainlink-solana", branch = "solana-2.1", rev = "<commit-hash>" }Also, minor style nit: consider using consistent syntax for dependencies (both use inline table
{ version = "..." }or both use string shorthand).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/sablier-common/Cargo.toml` around lines 10 - 13, The chainlink_solana git dependency in Cargo.toml currently points at the branch "solana-2.1" which risks non-reproducible builds; update the chainlink_solana entry to pin to a specific commit SHA or tag (e.g., replace branch = "solana-2.1" with rev = "<commit-sha>" or tag = "<tag-name>") so the build is deterministic, and while editing make the dependency syntax consistent with the other entries (use the same inline table form or string shorthand for anchor-lang, anchor-spl, and chainlink_solana) so formatting is uniform.crates/sablier-common/src/fee_calculation.rs (1)
35-36: Minor: Consider propagating the error instead of unwrapping.The
get_current_time().unwrap()on line 36 will panic ifClock::get()fails. While this is unlikely in a valid Solana runtime, the function already returns0for other error conditions, so handling this gracefully might be more consistent.🛡️ Suggested defensive handling
// Downcasting is safe as long as the date is before 7 February 2106 at 06:28:16 UTC. - let current_timestamp: u32 = get_current_time().unwrap() as u32; + let current_timestamp: u32 = match get_current_time() { + Ok(t) => t as u32, + Err(_) => return 0, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/sablier-common/src/fee_calculation.rs` around lines 35 - 36, The unwrap on get_current_time() (used to build current_timestamp) can panic; replace it by propagating the error from get_current_time() using the ? operator (update the enclosing function to return a Result) so current_timestamp is built like let current_timestamp: u32 = get_current_time()? as u32; if changing the signature is undesirable, fall back to a non-panicking default such as let current_timestamp: u32 = get_current_time().unwrap_or(0) as u32; adjust the function signature or error handling accordingly in fee_calculation.rs where get_current_time and current_timestamp are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/sablier-common/Cargo.toml`:
- Around line 10-13: The chainlink_solana git dependency in Cargo.toml currently
points at the branch "solana-2.1" which risks non-reproducible builds; update
the chainlink_solana entry to pin to a specific commit SHA or tag (e.g., replace
branch = "solana-2.1" with rev = "<commit-sha>" or tag = "<tag-name>") so the
build is deterministic, and while editing make the dependency syntax consistent
with the other entries (use the same inline table form or string shorthand for
anchor-lang, anchor-spl, and chainlink_solana) so formatting is uniform.
In `@crates/sablier-common/src/constants.rs`:
- Line 2: The constant LAMPORTS_PER_SOL is defined using a float cast ("1e9 as
u64"); change it to an integer literal for clarity and correctness (e.g., use
1_000_000_000u64) or, if available, re-export the canonical value from
solana_program::native_token::LAMPORTS_PER_SOL instead of redefining it; update
the definition of LAMPORTS_PER_SOL in constants.rs accordingly.
In `@crates/sablier-common/src/fee_calculation.rs`:
- Around line 35-36: The unwrap on get_current_time() (used to build
current_timestamp) can panic; replace it by propagating the error from
get_current_time() using the ? operator (update the enclosing function to return
a Result) so current_timestamp is built like let current_timestamp: u32 =
get_current_time()? as u32; if changing the signature is undesirable, fall back
to a non-panicking default such as let current_timestamp: u32 =
get_current_time().unwrap_or(0) as u32; adjust the function signature or error
handling accordingly in fee_calculation.rs where get_current_time and
current_timestamp are used.
In `@crates/sablier-common/src/fee_collection.rs`:
- Around line 19-20: Replace the panic-prone addition when computing
safe_minimum by using saturating_add instead of
rent_exempt_minimum.checked_add(SAFE_RENT_BUFFER_LAMPORTS).unwrap(); locate the
constant SAFE_RENT_BUFFER_LAMPORTS and the variable safe_minimum calculation and
change it to use rent_exempt_minimum.saturating_add(SAFE_RENT_BUFFER_LAMPORTS)
so the code matches the defensive pattern used elsewhere (e.g., the logic on
line 23).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlcrates/sablier-common/Cargo.tomlcrates/sablier-common/src/constants.rscrates/sablier-common/src/fee_calculation.rscrates/sablier-common/src/fee_collection.rscrates/sablier-common/src/lib.rscrates/sablier-common/src/time.rscrates/sablier-common/src/token_transfer.rsprograms/lockup/Cargo.tomlprograms/lockup/src/instructions/collect_fees.rsprograms/lockup/src/utils/constants.rsprograms/lockup/src/utils/mod.rsprograms/merkle_instant/Cargo.tomlprograms/merkle_instant/src/instructions/collect_fees.rsprograms/merkle_instant/src/utils/constants.rsprograms/merkle_instant/src/utils/fee_calculation.rsprograms/merkle_instant/src/utils/mod.rsprograms/merkle_instant/src/utils/time.rsprograms/merkle_instant/src/utils/transfer_helper.rs
💤 Files with no reviewable changes (3)
- programs/merkle_instant/src/utils/time.rs
- programs/merkle_instant/src/utils/transfer_helper.rs
- programs/merkle_instant/src/utils/fee_calculation.rs
|
@IaroslavMazur aren’t there more parts that could be added to a common crate? |
Managed to extract the Do you have anything else in mind? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/sablier-common/src/fee_collection.rs (1)
42-43: Consider usingsaturating_addinstead ofchecked_add().unwrap().While overflow is practically impossible here (rent-exempt minimum + 0.001 SOL buffer), using
saturating_addwould be more defensive and eliminate the panic path entirely.Suggested change
const SAFE_RENT_BUFFER_LAMPORTS: u64 = 1_000_000; // 0.001 SOL - let safe_minimum = rent_exempt_minimum.checked_add(SAFE_RENT_BUFFER_LAMPORTS).unwrap(); + let safe_minimum = rent_exempt_minimum.saturating_add(SAFE_RENT_BUFFER_LAMPORTS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/sablier-common/src/fee_collection.rs` around lines 42 - 43, Replace the panic-prone checked_add().unwrap() usage when computing safe_minimum with a saturating_add to avoid potential overflow panics; specifically, change the expression that sets safe_minimum (which currently uses rent_exempt_minimum.checked_add(SAFE_RENT_BUFFER_LAMPORTS).unwrap()) to use rent_exempt_minimum.saturating_add(SAFE_RENT_BUFFER_LAMPORTS), keeping the constant SAFE_RENT_BUFFER_LAMPORTS and variable name safe_minimum unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/sablier-common/src/fee_collection.rs`:
- Around line 42-43: Replace the panic-prone checked_add().unwrap() usage when
computing safe_minimum with a saturating_add to avoid potential overflow panics;
specifically, change the expression that sets safe_minimum (which currently uses
rent_exempt_minimum.checked_add(SAFE_RENT_BUFFER_LAMPORTS).unwrap()) to use
rent_exempt_minimum.saturating_add(SAFE_RENT_BUFFER_LAMPORTS), keeping the
constant SAFE_RENT_BUFFER_LAMPORTS and variable name safe_minimum unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/sablier-common/src/fee_collection.rsprograms/lockup/src/instructions/withdraw.rsprograms/merkle_instant/src/instructions/claim.rs
Depends on #312.
Closes #192