fix for issue 139 #197
fix for issue 139 #197
Conversation
| @@ -847,6 +849,21 @@ | |||
|
|
|||
| try | |||
| { | |||
| // Check if the FounderKey is in use and get the total keys used | |||
| var keyCheckResult = await FounderKeyService.CheckFounderKeyAsync(project.ProjectInfo.FounderKey); | |||
There was a problem hiding this comment.
Putting this in the CreatProjectTransaction is probably not the right thing to do, we must know much before this point if the key was already used
There was a problem hiding this comment.
We do rescan before creating,
But there's a scenario this scenario.
- Logged on browser A, B.
- start create browser A.
- Start create browser B.
- Create browser B.
- Create Browser A.
unless you put a rescan when creating, you won't know that there's another project created on the same key.
but also now that i think of it we will need to refresh the data in the create a project. (almost everything except project info no?)
I know that this is an edge case that is not expected (especially when we suspect that per user there will be probably only 1 project), but still this should be addressed in some way because we cannot have any loophole of over riding a project.
There was a problem hiding this comment.
We need to have a meeting about trusting the relays for knowing if the project was created or only look at the indexer to see if a transaciton exists
Bare in mind that we keep the nostr event id on the transaction so it is not possible to show false data there.
There was a problem hiding this comment.
Okay, then i'll wait with this PR until we do that,
We should align on how much we trust the relays, and on which instances we can trust it.
Also, according to that we should check if there are other places in angor that trust the relay instead of the indexer?
There was a problem hiding this comment.
this must be review by @DavidGershony for the Nostr calls.
But also I am sure we do the check in the right place
src/Angor/Client/Pages/Founder.razor
Outdated
| @(scanningForProjects ? "Scanning..." : "Create Project") | ||
| </span> | ||
| </button> | ||
| <div class="tooltip-container" data-bs-toggle="tooltip" title="@GetCreateButtonTooltip()"> |
There was a problem hiding this comment.
can you make a separate pr just for issue 166?
There was a problem hiding this comment.
done,
but there's also a check that is a corelated to 166 in createtransaction that checks how many keys are already existing, and if there are more than 15 then fail.
(because of an issue like listed above)
There was a problem hiding this comment.
i'll also add that its needed because disabling the button in founder.razor isn't good enough,
because people can "hack" and change the URL.
so there needs to be check in the createtransaction (or a check on oninitiallized in create.razor that doesn't show the create module if you have more than 15.)
|
|
||
| public async Task<bool> IsFounderKeyInUseAsync(string founderKey) | ||
| { | ||
| var keys = _walletStorage.GetFounderKeys(); |
There was a problem hiding this comment.
I'm not sure the relay is the right place to check, we need to check with the indexer if the transaction exists.
| Kinds = new[] { NostrKind.ApplicationSpecificData, NostrKind.Metadata }, | ||
| })); | ||
|
|
||
| await tcs.Task; // Wait for the end of stream |
There was a problem hiding this comment.
Why are we awaiting the tcs and not just allowing the callback thread to perfome the action we need?
There was a problem hiding this comment.
I did it because of this
https://learn.microsoft.com/en-us/dotnet/standard/asynchronous-programming-patterns/implementing-the-task-based-asynchronous-pattern
why shouldnt we need TaskCompletionSource to wait for end of steam?
either way copilot removed it 😉
|
|
||
| if (keyCheckResult.IsKeyInUse) | ||
| { | ||
| notificationComponent.ShowErrorMessage("The FounderKey is already in use. Please use a different key."); |
There was a problem hiding this comment.
In this case we need to "fix" the data in the local storage because the user can't select a founder key to use, perhaps add a message to rescan or wipe the storage and recover the wallet again.
| } | ||
|
|
||
|
|
||
| public class FounderKeyCheckResult |
There was a problem hiding this comment.
This class should be in the models folder
|
created by copilot, pretty neat. The pull request #197 titled "fix for issue 139" includes the following changes:
|
to be tested