Conversation
miladsoft
requested review from
DavidGershony and
Copilot
and removed request for
Copilot
| /// Indicates if the response is successful. | ||
| /// </summary> | ||
| [ArrayProperty(2)] | ||
| public bool IsSuccess { get; init; } |
There was a problem hiding this comment.
What is the difference between this and the Accepted property?
There was a problem hiding this comment.
This IsSuccess property can be removed as it was an additional property added for testing purposes. It was redundant with the existing Accepted property which already provides the same functionality. The IsSuccess property has now been removed to simplify the codebase and avoid duplication.
| using System.Threading.Tasks; | ||
| using Nostr.Client.Messages; | ||
|
|
||
| namespace Nostr.Client.NostrPow.Mining |
There was a problem hiding this comment.
I thing PoW is enough the whole namespace is nostr anyway
| var tags = originalEvent.Tags ?? new NostrEventTags(); | ||
|
|
||
| // Find the existing nonce tag if any | ||
| NostrEventTag? nonceTag = tags.FindFirstTag("nonce"); |
There was a problem hiding this comment.
I noticed you use the "nonce" everywhere, can you add it to the const strings on the tag instead? or a local const...
| // Set a current timestamp | ||
| miningEvent = new NostrEvent | ||
| { | ||
| Id = miningEvent.Id, |
There was a problem hiding this comment.
This is wrong, The id of a different event can't fit a new event
| Kind = miningEvent.Kind, | ||
| Tags = miningEvent.Tags, | ||
| Content = miningEvent.Content, | ||
| Sig = miningEvent.Sig |
There was a problem hiding this comment.
The sig can't fit an event with changed parameters
| if (achievedDifficulty >= difficulty) | ||
| { | ||
| // We found a valid nonce, return the mined event | ||
| return candidateEvent.DeepClone(id, candidateEvent.Sig); |
There was a problem hiding this comment.
The sig you are setting here is not valid, better to sign it again
| /// <summary> | ||
| /// Interface for Nostr event miners | ||
| /// </summary> | ||
| public interface IMiner |
There was a problem hiding this comment.
I guess we don't need this
| if (targetDifficulty.HasValue && targetDifficulty.Value < minDifficulty) | ||
| return false; | ||
|
|
||
| return true; |
There was a problem hiding this comment.
You also need to check that the difficulty is greater than or equal to the target difficulty
| /// </summary> | ||
| public static int CountLeadingZeroBitsInNibble(int nibble) | ||
| { | ||
| if (nibble >= 8) return 0; |
There was a problem hiding this comment.
return return count + BitOperations.LeadingZeroCount((uint)nibble) - 24;
| /// <summary> | ||
| /// Try to parse a hex character to its integer value | ||
| /// </summary> | ||
| private static bool TryParseHexDigit(char c, out int value) |
There was a problem hiding this comment.
Why use int if we only look at the char value?
| using System; | ||
| using System.Security.Cryptography; | ||
|
|
||
| namespace NostrDebug.Web.Pow |
There was a problem hiding this comment.
I'm not really sure, but why do we have a second implementation of the PoW instead of calling the implementation on the NostrEvent?
2 participants
No description provided.