Fix NullReferenceException in Web3.BlockHash() and AwaitWsRpcConnection()#261
Fix NullReferenceException in Web3.BlockHash() and AwaitWsRpcConnection()#261nikolaysamoil0ff wants to merge 3 commits intomagicblock-labs:mainfrom
Conversation
WalkthroughThe changes add null-handling safeguards across two runtime files: a guard clause in WalletBase that throws when WebSocket connection is uninitialized, and a nullable return type update in Web3's BlockHash method with corresponding async implementation adjustments. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Runtime/codebase/WalletBase.csRuntime/codebase/Web3.cs
🤖 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 392-395: The method AwaitWsRpcConnection currently throws
synchronously when _webSocketConnection is null; change it to return a faulted
Task instead so callers observe the error via TAP. In AwaitWsRpcConnection,
replace the synchronous throw with returning
Task.FromException<Task>(_webSocketConnection == null ? new
InvalidOperationException("WebSocket connection is not initialized.") : /* keep
existing return */) — more specifically return Task.FromException(new
InvalidOperationException("WebSocket connection is not initialized.")) when
_webSocketConnection is null, otherwise continue to return
_webSocketConnection.Task; this keeps AwaitWsRpcConnection non-async while
producing a faulted Task for uniform exception propagation.
In `@Runtime/codebase/Web3.cs`:
- Around line 319-321: The current static helper checks only Instance != null
but still dereferences Instance.WalletBase when calling
Instance.WalletBase.GetBlockHash(commitment, useCache, maxSeconds), which can
cause an NRE if WalletBase is null; update the guard to ensure WalletBase is
non-null (e.g., Instance != null && Instance.WalletBase != null or using a
null-conditional) and return Task.FromResult<string?>(null) when WalletBase is
null so the call to GetBlockHash is only invoked on a valid WalletBase instance.
- Line 315: The public API method BlockHash was changed from Task<string> to
Task<string?> to reflect that WalletBase.GetBlockHash actually returns null in
some cases (when Instance is null or blockhash is null), which is
source-breaking for nullable-aware consumers; update the declaration and any
related usages to return Task<string?> and ensure callers handle a possible null
result, and audit WalletBase.GetBlockHash, the BlockHash method, and any call
sites that use Instance or check blockhash to consistently propagate or handle
the nullable string return.
| if (_webSocketConnection == null) | ||
| throw new InvalidOperationException("WebSocket connection is not initialized."); | ||
|
|
||
| return _webSocketConnection.Task; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Return a faulted Task instead of throwing synchronously for uniform TAP exception propagation.
AwaitWsRpcConnection() is a non-async Task-returning method. A synchronous throw is observable differently from a faulted Task when callers don't immediately await the return value (e.g., Task.WhenAll, .ContinueWith, or storing the Task before awaiting). In cases like parallel processing (Task.WhenAll), the difference between throwing directly and returning a failed task becomes important.
♻️ Proposed refactor
- if (_webSocketConnection == null)
- throw new InvalidOperationException("WebSocket connection is not initialized.");
-
- return _webSocketConnection.Task;
+ return _webSocketConnection?.Task
+ ?? Task.FromException(new InvalidOperationException("WebSocket connection is not initialized."));📝 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.
| if (_webSocketConnection == null) | |
| throw new InvalidOperationException("WebSocket connection is not initialized."); | |
| return _webSocketConnection.Task; | |
| return _webSocketConnection?.Task | |
| ?? Task.FromException(new InvalidOperationException("WebSocket connection is not initialized.")); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/codebase/WalletBase.cs` around lines 392 - 395, The method
AwaitWsRpcConnection currently throws synchronously when _webSocketConnection is
null; change it to return a faulted Task instead so callers observe the error
via TAP. In AwaitWsRpcConnection, replace the synchronous throw with returning
Task.FromException<Task>(_webSocketConnection == null ? new
InvalidOperationException("WebSocket connection is not initialized.") : /* keep
existing return */) — more specifically return Task.FromException(new
InvalidOperationException("WebSocket connection is not initialized.")) when
_webSocketConnection is null, otherwise continue to return
_webSocketConnection.Task; this keeps AwaitWsRpcConnection non-async while
producing a faulted Task for uniform exception propagation.
| /// https://docs.solana.com/developing/transaction_confirmation#how-does-transaction-expiration-work</param> | ||
| /// <returns></returns> | ||
| public static Task<string> BlockHash( | ||
| public static Task<string?> BlockHash( |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for nullable enable in csproj and Directory.Build.props
rg -n "Nullable" --include="*.csproj" --include="*.props" --include="Directory.Build.*" -i
# Check for file-level nullable enable directives in C# files
rg -n "#nullable enable" --type=cs
# Also check the actual BlockHash method and its usages
echo "=== BlockHash method definition ==="
rg -n "BlockHash" --type=cs -A 2 -B 2
# Check if there are any callers of BlockHash
echo "=== BlockHash usages/callers ==="
rg -n "BlockHash\(" --type=csRepository: magicblock-labs/Solana.Unity-SDK
Length of output: 21917
🏁 Script executed:
#!/bin/bash
# Find and inspect .csproj files for Nullable settings
echo "=== Finding .csproj files ==="
find . -name "*.csproj" -type f | head -5
# Check the main csproj for Nullable setting
echo -e "\n=== Checking for Nullable in csproj files ==="
find . -name "*.csproj" -exec grep -l "Nullable" {} \;
# Check Web3.cs for file-level nullable directives and full method signature
echo -e "\n=== Web3.cs nullable directives and BlockHash method ==="
head -20 Runtime/codebase/Web3.cs
tail -n +310 Runtime/codebase/Web3.cs | head -20
# Check WalletBase.cs GetBlockHash implementation fully
echo -e "\n=== WalletBase GetBlockHash implementation ==="
sed -n '287,315p' Runtime/codebase/WalletBase.csRepository: magicblock-labs/Solana.Unity-SDK
Length of output: 3002
API change is breaking for external SDK consumers, but semantically necessary.
Changing Task<string> to Task<string?> is source-breaking for external callers with #nullable enable. However, this project itself doesn't enable nullable reference types, and the underlying implementation of WalletBase.GetBlockHash already returns null in multiple places despite its non-nullable return type declaration. This change makes the API contract accurately reflect the actual nullability behavior (when Instance is null or when blockhash is null). While external SDK consumers will need to adjust their type handling, this is a necessary correction rather than an unintended breaking change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/codebase/Web3.cs` at line 315, The public API method BlockHash was
changed from Task<string> to Task<string?> to reflect that
WalletBase.GetBlockHash actually returns null in some cases (when Instance is
null or blockhash is null), which is source-breaking for nullable-aware
consumers; update the declaration and any related usages to return Task<string?>
and ensure callers handle a possible null result, and audit
WalletBase.GetBlockHash, the BlockHash method, and any call sites that use
Instance or check blockhash to consistently propagate or handle the nullable
string return.
| Instance != null | ||
| ? Instance.WalletBase.GetBlockHash(commitment, useCache, maxSeconds) | ||
| : Task.FromResult<string?>(null); |
There was a problem hiding this comment.
Instance.WalletBase is not null-guarded — NRE still possible after logout or before wallet initialization.
The guard on line 319 only checks Instance != null. When Instance exists but Instance.WalletBase is null (e.g., after Logout() sets WalletBase = null, or before any wallet is set), line 320 dereferences a null reference and throws NullReferenceException. All other static helpers in this class (Rpc, WsRpc, Account) guard against both nulls — this one should too.
🐛 Proposed fix
- Instance != null
- ? Instance.WalletBase.GetBlockHash(commitment, useCache, maxSeconds)
- : Task.FromResult<string?>(null);
+ Instance != null && Instance.WalletBase != null
+ ? Instance.WalletBase.GetBlockHash(commitment, useCache, maxSeconds)
+ : Task.FromResult<string?>(null);📝 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.
| Instance != null | |
| ? Instance.WalletBase.GetBlockHash(commitment, useCache, maxSeconds) | |
| : Task.FromResult<string?>(null); | |
| Instance != null && Instance.WalletBase != null | |
| ? Instance.WalletBase.GetBlockHash(commitment, useCache, maxSeconds) | |
| : Task.FromResult<string?>(null); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Runtime/codebase/Web3.cs` around lines 319 - 321, The current static helper
checks only Instance != null but still dereferences Instance.WalletBase when
calling Instance.WalletBase.GetBlockHash(commitment, useCache, maxSeconds),
which can cause an NRE if WalletBase is null; update the guard to ensure
WalletBase is non-null (e.g., Instance != null && Instance.WalletBase != null or
using a null-conditional) and return Task.FromResult<string?>(null) when
WalletBase is null so the call to GetBlockHash is only invoked on a valid
WalletBase instance.
1 participant
Problem
Web3.BlockHash()could return null instead of a Task, causingNullReferenceExceptionwhen awaited.WalletBase.AwaitWsRpcConnection()could access_webSocketConnection.Taskwhile_webSocketConnectionis null, causingNullReferenceException.Solution
Web3.BlockHash()now returns a completedTask<string?>with null result if Instance is not initialized.Added explicit null check in
AwaitWsRpcConnection()to throw a clearInvalidOperationExceptionif_webSocketConnectionis not initialized.Before & After Screenshots
Insert screenshots of example code output
BEFORE:
AFTER:
Other changes (e.g. bug fixes, small refactors)
N/A
Deploy Notes
Notes regarding deployment of the contained body of work. These should note any
new dependencies, new scripts, etc.
New scripts:
script: script detailsNew dependencies:
dependency: dependency detailsSummary by CodeRabbit
Bug Fixes
Changes