Skip to content

refactor(key-wallet-ffi): drop unused managed_wallet_check_transaction#684

Draft
ZocoLini wants to merge 1 commit intov0.42-devfrom
fix/vec-deallocation
Draft

refactor(key-wallet-ffi): drop unused managed_wallet_check_transaction#684
ZocoLini wants to merge 1 commit intov0.42-devfrom
fix/vec-deallocation

Conversation

@ZocoLini
Copy link
Copy Markdown
Collaborator

I was investigating an issue where a Vec's ownership was being taken back by Rust, but reconstructing it with Vec::from_raw_parts using the stored len as capacity is UB. My idea was to implement Drop and use fixed slices instead of vectors, but then I realized we're assigning a value to a pointed structure without exposing a way to allocate it. Since it didn't make sense, no platform package is actually using it, and I'm not willing to allocate memory on the C side breaking our own patterns, I just dropped the whole thing

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 24, 2026

Warning

Rate limit exceeded

@ZocoLini has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 45 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 45 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 45a81eed-8627-4f84-bdee-db3debdeb37b

📥 Commits

Reviewing files that changed from the base of the PR and between 0093278 and f55b1a2.

📒 Files selected for processing (2)
  • key-wallet-ffi/FFI_API.md
  • key-wallet-ffi/src/transaction_checking.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/vec-deallocation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.53%. Comparing base (0093278) to head (f55b1a2).
⚠️ Report is 4 commits behind head on v0.42-dev.

Additional details and impacted files
@@              Coverage Diff              @@
##           v0.42-dev     #684      +/-   ##
=============================================
+ Coverage      70.22%   70.53%   +0.30%     
=============================================
  Files            319      319              
  Lines          66686    66388     -298     
=============================================
- Hits           46830    46824       -6     
+ Misses         19856    19564     -292     
Flag Coverage Δ
core 75.81% <ø> (ø)
ffi 46.73% <ø> (+1.54%) ⬆️
rpc 20.00% <ø> (ø)
spv 86.46% <ø> (-0.05%) ⬇️
wallet 68.79% <ø> (ø)
Files with missing lines Coverage Δ
key-wallet-ffi/src/transaction_checking.rs 21.42% <ø> (+19.58%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Copy Markdown
Member

@QuantumExplorer QuantumExplorer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No... we should not remove this.

@ZocoLini
Copy link
Copy Markdown
Collaborator Author

@QuantumExplorer and how are you planning to use it, there is no constructor for the C side, I can go back to my original idea of fixing the UB problem and create a constructor for the structure, but I need to know if you are gonna use this or not and where, I don't want to maintain thing just because I can

@github-actions
Copy link
Copy Markdown
Contributor

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

@github-actions github-actions Bot added the merge-conflict The PR conflicts with the target branch. label Apr 25, 2026
@ZocoLini ZocoLini marked this pull request as draft May 7, 2026 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-conflict The PR conflicts with the target branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants