feat(WalletBase): add SignMessage(string) convenience overload#274
feat(WalletBase): add SignMessage(string) convenience overload#274mstevens843 wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdded a UTF‑8 string overload for SignMessage: new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
mstevens843
force-pushed
the
feat/sign-message-string-overload
branch
from
dcbbef1 to
e7d3f9a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Runtime/codebase/WalletBase.cs`:
- Around line 273-277: The new WalletBase.SignMessage(string) overload can cause
compile-time ambiguity when callers invoke SignMessage(null) because null is
convertible to both string and byte[]; to fix, remove the ambiguous overload or
rename it to a non-conflicting public name (e.g., WalletBase.SignUtf8Message)
and update any internal call sites to use the new name; if you must keep the
overload, add clear XML documentation on WalletBase.SignMessage(string) warning
about the null ambiguity and prefer the byte[] overload, but the preferred fix
is renaming to SignUtf8Message to avoid CS0121 for external consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e429ab7d-1543-4f2f-845a-5b5e7f289c73
📒 Files selected for processing (1)
Runtime/codebase/WalletBase.cs
|
Pushed a doc-only update for the CodeRabbit nitpick. Kept the SignMessage(string) overload since it follows the standard C# overload convention developers expect, and the runtime null guard already throws ArgumentNullException. Added an XML block on the method explaining that a literal SignMessage(null) Ready for review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Runtime/codebase/IWalletBase.cs`:
- Around line 108-113: Remove the newly added string overload from the public
interface IWalletBase and provide it as an extension method (or keep it only on
WalletBase) so external implementers are not forced to implement it;
specifically, delete Task<byte[]> SignMessage(string message) from IWalletBase
and add a public static Task<byte[]> SignMessage(this IWalletBase wallet, string
message) extension that encodes the string to UTF‑8 and delegates to the
existing SignMessage(byte[] ...) method, or alternatively implement the string
overload as an instance method only on the concrete WalletBase class to keep the
interface unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68c75a5b-a927-43a4-8c16-1e23aa03ffaf
📒 Files selected for processing (2)
Runtime/codebase/IWalletBase.csRuntime/codebase/WalletBase.cs
…Base Adds a UTF-8 string convenience overload via an extension method on IWalletBase, so callers can sign plain strings without manual UTF-8 encoding. Implemented as an extension to avoid the interface-expansion breakage CodeRabbit flagged on the prior version (a required member on IWalletBase would have forced every external implementer to add the method to their own classes). Instance methods take priority over extension methods, so wallet.SignMessage(null) still binds unambiguously to SignMessage(byte[]) and the runtime null guard already handles that path. The string extension throws ArgumentNullException for null inputs.
mstevens843
force-pushed
the
feat/sign-message-string-overload
branch
from
3f03ca1 to
6b9886f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Problem
SignMessage()only acceptsbyte[]. Every caller must manually convert strings viaEncoding.UTF8.GetBytes()before signing, adding unnecessary boilerplate for themost common use case — signing human-readable messages.
Discovered while building a Unity MWA example app on Solana Seeker hardware with Seed Vault
(unity-solana-mwa-example). Documented as Point 6 in #273.
Solution
Added
SignMessage(string message)overload toIWalletBaseandWalletBasethat handles UTF-8 encoding internally, delegating to the existingSignMessage(byte[]).All derived wallet classes (
InGameWallet,SolanaMobileWalletAdapter,SolanaWalletAdapterWebGL,PhantomDeepLink,SolanaWalletAdapter) inherit the new overloadautomatically — zero changes required in implementations.
Before & After Screenshots
BEFORE:
AFTER:
Other changes (e.g. bug fixes, small refactors)
None. Purely additive — no existing signatures or behavior modified.
Deploy Notes
No new scripts or dependencies. Drop-in enhancement to existing API surface.
Summary by CodeRabbit