Conversation
WalkthroughThis PR updates the anchor-lang dependency to version 0.31.0, refactors the Session derive macro to support both V1 and V2 session token variants through conditional logic, adds a backward compatibility alias, and reverses the expiration check logic in session token validation functions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
programs/gpl_session/macros/attribute/src/lib.rs (1)
82-104: Clarify macro naming and documentation.The macro is now named
SessionV2(line 83), but the comment says it "supports both SessionToken and SessionTokenV2" (line 82). This is confusing because:
- The macro name suggests it's only for V2
- The derive actually generates either
SessionorSessionV2trait depending on the field typeConsider updating the comment to clarify that the macro name is
SessionV2but it automatically detects and supports both token types.Apply this diff:
-// Macro to derive Session Trait (supports both SessionToken and SessionTokenV2) +// Macro to derive Session traits - automatically detects the token type and generates +// either Session (for SessionToken) or SessionV2 (for SessionTokenV2) trait implementation. #[proc_macro_derive(SessionV2, attributes(session))] pub fn derive(input: TokenStream) -> TokenStream {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
programs/gpl_session/Cargo.toml(1 hunks)programs/gpl_session/macros/attribute/src/lib.rs(4 hunks)programs/gpl_session/src/lib.rs(1 hunks)
🔇 Additional comments (4)
programs/gpl_session/macros/attribute/src/lib.rs (3)
30-80: LGTM - Token type detection is thorough.The
SessionTokenTypeenum andget_session_token_type()function correctly identify whether a field is of typeOption<Account<'info, SessionToken>>(V1) orOption<Account<'info, SessionTokenV2>>(V2). The nested type checking is comprehensive.
123-176: LGTM - Trait implementations are correctly generated.The match statement correctly generates:
Sessiontrait implementation withSessionTokentype for V1SessionV2trait implementation withSessionTokenV2type for V2Both implementations provide the required methods:
target_program(),session_token(),session_authority(), andsession_signer().
198-201: LGTM - Documentation updated for dual-variant support.The comment correctly indicates that the
session_auth_ormacro works with bothSessionandSessionV2traits, which is accurate given that it calls generic methods likesession_token(),is_valid(), andsession_authority()that are implemented by both traits.programs/gpl_session/Cargo.toml (1)
23-23: anchor-lang version 0.31.0 is valid and free from known vulnerabilities.Verification confirms that version 0.31.0 exists, was released on March 8–9, 2025, and is available on crates.io. No publicly disclosed CVEs or high-severity security advisories were found for this version. Notably, v0.31.0 includes significant security improvements addressing stack-memory overflow risks and dependency compatibility issues with solana-program. The pinned version is appropriate for use.
| // Macro to derive SessionV2 Trait (alias for Session macro for backwards compatibility) | ||
| #[proc_macro_derive(Session, attributes(session))] | ||
| pub fn derive_v2(input: TokenStream) -> TokenStream { | ||
| derive(input) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify backward compatibility comment.
The comment on line 181 states "alias for Session macro for backwards compatibility", but this is misleading. The derive_v2 function makes Session an alias that calls the same derive function as SessionV2, rather than the other way around.
Both #[derive(Session)] and #[derive(SessionV2)] now invoke the same implementation that auto-detects the token type. This means existing code using #[derive(Session)] with SessionToken fields will continue to work.
Consider clarifying the comment.
Apply this diff:
-// Macro to derive SessionV2 Trait (alias for Session macro for backwards compatibility)
+// Backward compatibility: #[derive(Session)] now delegates to the same implementation
+// as #[derive(SessionV2)], auto-detecting whether to generate Session or SessionV2 trait.
#[proc_macro_derive(Session, attributes(session))]
pub fn derive_v2(input: TokenStream) -> TokenStream {
derive(input)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Macro to derive SessionV2 Trait (alias for Session macro for backwards compatibility) | |
| #[proc_macro_derive(Session, attributes(session))] | |
| pub fn derive_v2(input: TokenStream) -> TokenStream { | |
| derive(input) | |
| } | |
| // Backward compatibility: #[derive(Session)] now delegates to the same implementation | |
| // as #[derive(SessionV2)], auto-detecting whether to generate Session or SessionV2 trait. | |
| #[proc_macro_derive(Session, attributes(session))] | |
| pub fn derive_v2(input: TokenStream) -> TokenStream { | |
| derive(input) | |
| } |
🤖 Prompt for AI Agents
In programs/gpl_session/macros/attribute/src/lib.rs around lines 181 to 185, the
comment for derive_v2 is misleading; update it to clarify that derive_v2
implements the same derive function as SessionV2 and that both
#[derive(Session)] and #[derive(SessionV2)] now delegate to the same
implementation which auto-detects token types so existing uses of
#[derive(Session)] continue to work; replace the current wording with a concise
note stating that Session is an alias that forwards to the shared derive
implementation (same as SessionV2) for backward compatibility and token
auto-detection.
| pub fn is_expired(&self) -> Result<bool> { | ||
| let now = Clock::get()?.unix_timestamp; | ||
| Ok(now < self.valid_until) | ||
| Ok(now > self.valid_until) | ||
| } |
There was a problem hiding this comment.
Critical: Inverted expiration logic breaks V2 validation.
The is_expired() method returns true when now > self.valid_until (when expired), but validate() at line 481 directly returns is_expired(), and is_valid() calls validate(). This means is_valid() will return true when the token is expired, which is incorrect.
At line 218, the code requires is_valid() to be true: require!(ctx.accounts.is_valid()?, SessionError::InvalidToken). With the current logic, this will accept expired tokens and reject valid ones.
Note that V1's is_expired() (line 425) has the opposite behavior: it returns true when now < valid_until (when NOT expired). While the V1 naming is incorrect, the behavior is correct because validate() returns this value.
Apply this diff to fix the logic:
impl SessionTokenV2 {
pub fn is_expired(&self) -> Result<bool> {
let now = Clock::get()?.unix_timestamp;
- Ok(now > self.valid_until)
+ Ok(now < self.valid_until)
}Or, for better clarity, rename the method to is_valid() and update all call sites:
impl SessionTokenV2 {
- pub fn is_expired(&self) -> Result<bool> {
+ pub fn is_valid(&self) -> Result<bool> {
let now = Clock::get()?.unix_timestamp;
- Ok(now > self.valid_until)
+ Ok(now < self.valid_until)
}
// validate the token
pub fn validate(&self, ctx: ValidityCheckerV2) -> Result<bool> {
let target_program = ctx.target_program;
let session_signer = ctx.session_signer.key();
let authority = ctx.authority.key();
// Check the PDA seeds
let seeds = &[
SessionTokenV2::SEED_PREFIX.as_bytes(),
target_program.as_ref(),
session_signer.as_ref(),
authority.as_ref(),
];
let (pda, _) = Pubkey::find_program_address(seeds, &crate::id());
require_eq!(pda, ctx.session_token.key(), SessionError::InvalidToken);
// Check if the token has expired
- self.is_expired()
+ self.is_valid()
}
}And update line 378:
pub fn revoke_session_token_handler_v2(ctx: Context<RevokeSessionTokenV2>) -> Result<()> {
// If the session is still active, the authority must be a signer
- if !ctx.accounts.session_token.is_expired()? {
+ if ctx.accounts.session_token.is_valid()? {
require!(ctx.accounts.authority.is_signer, SessionError::InvalidAuthority);
}
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn is_expired(&self) -> Result<bool> { | |
| let now = Clock::get()?.unix_timestamp; | |
| Ok(now < self.valid_until) | |
| Ok(now > self.valid_until) | |
| } | |
| pub fn is_expired(&self) -> Result<bool> { | |
| let now = Clock::get()?.unix_timestamp; | |
| Ok(now < self.valid_until) | |
| } |
🤖 Prompt for AI Agents
programs/gpl_session/src/lib.rs around lines 457-460: the current is_expired()
returns true when now > valid_until (i.e. when expired) but callers expect a
truthy value for valid tokens, causing expired tokens to be accepted; change the
method so it returns false when expired and true when still valid (e.g., return
Ok(now < self.valid_until)) or rename the method to is_valid() and invert its
logic, then update all call sites accordingly (including the call at line 378
and the validate()/is_valid() wrappers) so they check the corrected boolean
semantics.
1 participant
Problem
#9 introduced V2 instructions but didnt have macros that could be used in the contract.
Solution
Add macros to support V2 instructions similar to V1.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.