Skip to content

Comments

Eliminate warnings from the solution build#670

Closed
dangershony wants to merge 6 commits intomainfrom
eliminate-warnings
Closed

Eliminate warnings from the solution build#670
dangershony wants to merge 6 commits intomainfrom
eliminate-warnings

Conversation

@dangershony
Copy link
Member

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request eliminates compiler warnings from the solution build by addressing nullable reference type warnings and other compiler diagnostics. The changes include adding null-forgiving operators (!), initializing properties with default values, converting .Value to GetValueOrDefault(), removing duplicate using statements, and selectively suppressing specific warnings where appropriate.

Changes:

  • Added null-forgiving operators throughout the codebase to handle nullable reference types
  • Initialized properties with default values (string.Empty, null!, [], etc.) to satisfy non-nullable requirements
  • Converted nullable DateTime .Value access to GetValueOrDefault() for safer null handling
  • Suppressed specific warnings (CS0649, CS0414, CS8669, CS9113, CS1998, CS8620, CS0067, CS8601) with documentation
  • Removed duplicate using statements
  • Fixed async/await patterns where methods were incorrectly marked async

Reviewed changes

Copilot reviewed 132 out of 132 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
Directory.Build.props Added CS8669 warning suppression for Avalonia project
WalletOperations.cs Added null-forgiving operators for coins collection
AssemblyInfoHelper.cs Initialized string properties with empty defaults
SignService.cs Added null-forgiving operators and converted to GetValueOrDefault()
RelaySubscriptionsHandling.cs Added null-forgiving operators for event responses
RelayService.cs Added null-forgiving operators for Nostr events and IDs
NostrCommunicationFactory.cs Added null-forgiving operators for relay URLs
NetworkService.cs Initialized event handler and changed nullable network declaration
MempoolSpaceIndexerApi.cs Initialized properties with default values
IndexerService.cs Added logger field with CS0649 suppression (obsolete class)
PsbtOperations.cs Added null-forgiving operator for change address
InvestmentTransactionBuilder.cs Changed nullable Script declaration
TaprootScriptBuilder.cs Changed nullable Script declaration
InvestmentScriptBuilder.cs Changed .Value to GetValueOrDefault()
Networks.cs Added null-forgiving operator for null network returns
BitcoinSignet.cs, BitcoinMain.cs Added null-forgiving operators for template lookups
Multiple model files Initialized properties with default values (string.Empty, null!, [])
Multiple Avalonia UI files Initialized properties and added null-forgiving operators
TransactionHistory.cs Added CS9113 suppression for reserved logger parameter
FileStore.cs Added CS1998 suppression (incorrectly documented)
Multiple SDK operations Added CS9113 suppressions for DI-injected unused parameters
Multiple test files Fixed type casting with explicit long conversions and null-forgiving operators
Various files Removed duplicate using statements

Comment on lines 23 to +29
public async Task<Result> Save<T>(string key, T data)
{
return from filePath in Result.Try(() => Path.Combine(appDataPath, key))
from contents in Result.Try(() => JsonSerializer.Serialize(data, new JsonSerializerOptions { WriteIndented = true }))
select Result.Try(() => File.WriteAllTextAsync(filePath, contents))
.Bind(Result.Success);
#pragma warning restore CS1998
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

The File.WriteAllTextAsync method is called but not awaited on line 27. This means the method will return before the file write completes, which could lead to data loss or corruption. The method should either: 1) await the WriteAllTextAsync call and return its result, or 2) use the synchronous File.WriteAllText method. Also, the pragma warning comment is misleading - the async keyword is needed to match the interface signature and properly await async operations, not for "LINQ query expression type inference".

Copilot uses AI. Check for mistakes.
nostrClient.Send(new NostrEventRequest(signed));

return Task.FromResult(signed.Id);
return Task.FromResult(signed.Id)!;
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

Incorrect placement of null-forgiving operator. The null-forgiving operator should be on signed.Id rather than on Task.FromResult. Change Task.FromResult(signed.Id)! to Task.FromResult(signed.Id!). The same issue exists on line 264.

Copilot uses AI. Check for mistakes.
nostrClient.Send(new NostrEventRequest(signed));

return Task.FromResult(signed.Id);
return Task.FromResult(signed.Id)!;
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

Incorrect placement of null-forgiving operator. The null-forgiving operator should be on signed.Id rather than on Task.FromResult. Change Task.FromResult(signed.Id)! to Task.FromResult(signed.Id!). This is the same issue as on line 242.

Copilot uses AI. Check for mistakes.
eventId!,
nostrEvent.Pubkey!,
nostrEvent.Content!,
nostrEvent.CreatedAt!.Value);
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

Inconsistent handling of nullable DateTime. This line uses CreatedAt!.Value while similar code throughout this file was changed to use GetValueOrDefault() (see lines 124, 160, 221, 256, 464). Consider changing this to nostrEvent.CreatedAt.GetValueOrDefault() for consistency and to handle null values safely.

Copilot uses AI. Check for mistakes.
NostrEvent = nostrEvent!,
ProfileIdentifier = nostrEvent.Tags!.FindFirstTagValue(NostrEventTag.ProfileIdentifier) ?? string.Empty,
EventCreatedAt = nostrEvent.CreatedAt.GetValueOrDefault(),
EventIdentifier = nostrEvent.Tags.FindFirstTagValue(NostrEventTag.EventIdentifier) ?? string.Empty
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

Missing null-forgiving operator on nostrEvent.Tags. Line 463 uses nostrEvent.Tags!.FindFirstTagValue(...) but line 465 uses nostrEvent.Tags.FindFirstTagValue(...) without the null-forgiving operator. This is inconsistent and may cause a compiler warning.

Copilot uses AI. Check for mistakes.

var secretHash = uint256.Parse("e1449d99d4861ff66a41e316d5605a37d79168a954b4f9cd0bb4656c7cd5dcfc");
var investorKey = "03eb7d47c80390672435987b9a7ecaa22730cd9c4537fc8d257417fb058248ed77";
//var investorKey = "03eb7d47c80390672435987b9a7ecaa22730cd9c4537fc8d257417fb058248ed77";
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

Commented-out variable that appears to be unused. If this variable is truly not needed, consider removing it entirely rather than leaving it commented out. Dead code should be removed to keep the codebase clean.

Copilot uses AI. Check for mistakes.
Comment on lines 223 to 240
@@ -230,12 +232,12 @@ public async Task<string> GetTransactionHexByIdAsync(string transactionId)
return (true, blockHashElement.GetString());
}

_logger.LogWarning("blockHash not found in the block response.");
_logger!.LogWarning("blockHash not found in the block response.");
return (true, null); // Indexer is online, but no valid block hash
}
catch (Exception ex)
{
_logger.LogError(ex, $"Error during indexer network check: {ex.Message}");
_logger!.LogError(ex, $"Error during indexer network check: {ex.Message}");
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

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

Potential NullReferenceException. The _logger field is never assigned (it's not injected in the constructor) and is marked as nullable, but it's used with null-forgiving operators on lines 223, 235, and 240. This will throw a NullReferenceException at runtime. Since this class is marked as obsolete, consider either: 1) removing the logger calls entirely, 2) checking for null before using it, or 3) injecting it in the constructor.

Copilot uses AI. Check for mistakes.
@dangershony
Copy link
Member Author

I will submit a new PR that fixes warnings that are safe only

Minor/Good Changes
- int → long casts on NBitcoin.Money(1000L) — safe, just disambiguating constructor overloads
- Duplicate using removals — good cleanup
- await added to walletStore.SaveAll — actually a bug fix (was fire-and-forget before)
- Null guards on Nostr event subscriptions — good, but silently drops null events
- CS8669 suppressed globally in Directory.Build.props — worth noting, hides future nullability mismatches project-wide
---
Summary
The most dangerous pattern in this PR is replacing .Value with .GetValueOrDefault() on nullable types in protocol/money code. This turns loud failures into silent corruption. Items 1 and 2 are the highest risk — they could produce wrong Bitcoin script timelocks.
The widespread null! usage doesn't fix nullability issues; it just moves the crash from compile-time awareness to runtime surprise. The PR would benefit from proper null handling (null checks with early returns/error results) instead of null! suppression, especially in the protocol and transaction-building layers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant