-
Notifications
You must be signed in to change notification settings - Fork 23
feat: ActionArgs::default() to abstract escrow_index #662
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
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
WalkthroughAdded an inherent impl block for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-magic-program-api/src/args.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (3)
magicblock-magic-program-api/src/args.rs (3)
22-24: Getter is redundant but acceptable for future-proofing.Since
escrow_indexis a public field (line 11), this getter is technically redundant. However, it may be useful if you plan to make the field private in the future or add validation logic.
30-33: Builder pattern implementation is correct and approved.Verification confirms this is the only builder-style method in the codebase. The implementation correctly consumes
self, updates the field, and returnsselffor method chaining.
16-21: Renamedefaulttonewto avoid confusion with theDefaulttrait.The method name
defaultmisleadingly suggests the standardDefaulttrait, which has a different signature (no parameters). Since this is a custom constructor takingdataas a parameter, renaming tonewaligns with Rust conventions.Additionally, document the magic number
255forescrow_index. Extract it to a named constant or add a doc comment explaining its semantic meaning (e.g., whether it represents "no escrow" or "unset").+ /// Creates a new ActionArgs with the given data and a default escrow_index of 255 (no escrow selected). - pub fn default(data: Vec<u8>) -> Self { + pub fn new(data: Vec<u8>) -> Self { Self { escrow_index: 255, data, } }Since no existing call sites were found, this rename poses no integration risk.
| pub fn data(&self) -> &Vec<u8> { | ||
| &self.data | ||
| } |
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.
🛠️ Refactor suggestion | 🟠 Major
Return &[u8] instead of &Vec<u8> for idiomatic Rust.
Returning &Vec<u8> is considered an anti-pattern in Rust because it exposes the implementation detail and limits flexibility. Instead, return &[u8], which works with vectors, arrays, and slices. Callers can still obtain &Vec<u8> via deref coercion if needed.
Apply this diff:
- pub fn data(&self) -> &Vec<u8> {
- &self.data
+ pub fn data(&self) -> &[u8] {
+ &self.data
}📝 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 data(&self) -> &Vec<u8> { | |
| &self.data | |
| } | |
| pub fn data(&self) -> &[u8] { | |
| &self.data | |
| } |
🤖 Prompt for AI Agents
In magicblock-magic-program-api/src/args.rs around lines 26 to 28, the accessor
currently returns &Vec<u8> which leaks the implementation; change the method
signature to return a slice (&[u8]) instead and return a slice of the vector
(e.g., via as_slice() or a full-range slice) so callers get a &[u8] while the
internal field remains Vec<u8>.
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.