issue-274 #278

Open
m wants to merge 23 commits from issue-274 into staging
Owner

Closes #274

Tasks

  • Create diagrams
  • Put hardcoded blossom sigit in the CONFIG.YML
  • When adding a Blossom server, send a request to check if it's a valid Blossom server, and check the "title" tag if it includes "Blossom Server"
  • Display spinners, and say status (validating blossom server, signing auth event, uploading to blossom.sigit.io)
  • When getting appData get from all Blossom servers
  • When the user adds Blossom servers, updateAppData will send user app data to all Blossom servers
  • In settings add the "servers" page to add preferred blossom servers, the default will be blossom.sigit.io
  • Use the sigit_design for settings
  • When servers are found in meta.json (zipUrls) use those servers just to read from it
  • Creator: when uploading files, we should try to write to all servers, and only put the successful ones in the meta.json
  • Add zipUrls to the meta.json - location of the encrypted zip files, it can be on multiple file servers
  • Start checking zipUrls and use the first which has the correct hash
  • Fetch sigits from all blossom servers and display them
  • When you open /verify/sigitId, show on which blossom URL was found
  • If not published to other Blossom urls, publish to all user-preferred Blossom servers, when the user hits SIGN
  • Max 3 blossom servers
  • test blossom branch against a fresh account
  • When open verify or sign page show on which blossoms is uploaded
  • When visiting a SIGIT by a link that no longer has files from Blossom, we should display the metadata. Currently, the app breaks.

Issues introduced by refactor

  • When multiple signer markConfig list does not include second signer marks - (happened only once)

Deployed version

https://test.sigit.io/

Diagrams

image

Closes #274 ### Tasks - [x] Create diagrams - [x] Put hardcoded blossom sigit in the CONFIG.YML - [x] When adding a Blossom server, send a request to check if it's a valid Blossom server, and check the "title" tag if it includes "Blossom Server" - [x] Display spinners, and say status (validating blossom server, signing auth event, uploading to blossom.sigit.io) - [x] When getting `appData` get from all Blossom servers - [x] When the user adds Blossom servers, updateAppData will send user app data to all Blossom servers - [x] In settings add the "servers" page to add preferred blossom servers, the default will be blossom.sigit.io - [x] Use the sigit_design for settings - [x] When servers are found in meta.json (zipUrls) use those servers just to read from it - [x] Creator: when uploading files, we should try to write to all servers, and only put the successful ones in the meta.json - [x] Add `zipUrls` to the meta.json - location of the encrypted zip files, it can be on multiple file servers - [x] Start checking `zipUrls` and use the first which has the correct hash - [x] Fetch sigits from all blossom servers and display them - [x] When you open /verify/sigitId, show on which blossom URL was found - [x] If not published to other Blossom urls, publish to all user-preferred Blossom servers, when the user hits SIGN - [x] Max 3 blossom servers - [x] test blossom branch against a fresh account - [x] When open verify or sign page show on which blossoms is uploaded - [x] When visiting a SIGIT by a link that no longer has files from Blossom, we should display the metadata. Currently, the app breaks. ### Issues introduced by refactor - [x] When multiple signer markConfig list does not include second signer marks - (happened only once) ### Deployed version https://test.sigit.io/ ### Diagrams ![image](/attachments/db4c7b94-a0b8-4ea3-a56d-a843e31f3c2f)
1.3 MiB
m added 2 commits 2024-12-17 20:11:13 +00:00
y reviewed 2024-12-18 05:28:46 +00:00
@ -0,0 +80,4 @@
// Check if new server is a valid URL
if (
!/^(https?:\/\/)(?!-)([A-Za-z0-9-]{1,63}\.)+[A-Za-z]{2,6}$/gim.test(
Owner

should be a utility

should be a utility
m marked this conversation as resolved
m added 2 commits 2024-12-18 15:19:41 +00:00
feat: added config.json and fileServerMap to the redux
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 45s
3de5edfbf6
m added 1 commit 2024-12-23 11:29:25 +00:00
Merge branch 'staging' into issue-274
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 43s
185f24c046
m added 3 commits 2024-12-23 13:03:31 +00:00
m added 2 commits 2024-12-30 16:26:23 +00:00
fix: lint type fix
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 39s
d0d84a860f
m added 1 commit 2024-12-30 16:27:25 +00:00
Merge branch 'staging' into issue-274
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 46s
6dab22a495
m added 2 commits 2025-01-02 09:49:52 +00:00
chore(git): Merge branch 'staging' into issue-274
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 45s
18c07556ea
m added 1 commit 2025-01-02 12:32:30 +00:00
feat: maximum of 3 blossom servers
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 46s
67d33e1aff
m changed title from WIP: issue-274 to issue-274 2025-01-02 16:39:27 +00:00
Author
Owner

It needs to be thoroughly tested: https://test.sigit.io

It needs to be thoroughly tested: https://test.sigit.io
m added 2 commits 2025-01-09 08:29:47 +00:00
m added 1 commit 2025-01-13 11:13:20 +00:00
chore(git): Merging NDK refactor
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 39s
6777e96d23
eugene reviewed 2025-01-14 08:46:06 +00:00
@ -45,0 +43,4 @@
console.info(
`${file.name} uploaded to following file storages: ${urls.join(', ')}`
)
// This bit was returning an url, and return of this function is being set to mark.value, so it kind of
Owner

Can you please re-phrease this comment to make it clearer? Or better yet, add a comment to the overall function.

Can you please re-phrease this comment to make it clearer? Or better yet, add a comment to the overall function.
m marked this conversation as resolved
eugene reviewed 2025-01-14 08:53:19 +00:00
@ -156,21 +172,32 @@ const PdfMarking = (props: PdfMarkingProps) => {
<PdfView
currentFile={currentFile}
files={files}
Owner

Is it the case that if there are no files found in the zip, files will be an empty array? Can we rely on the length of the files array instead of introducing the noFiles flag?

Can you also add some comments, here and in the code, to clarify what the noFiles flag is meant to do?

Is it the case that if there are no files found in the zip, `files` will be an empty array? Can we rely on the length of the files array instead of introducing the `noFiles` flag? Can you also add some comments, here and in the code, to clarify what the `noFiles` flag is meant to do?
m marked this conversation as resolved
eugene reviewed 2025-01-14 09:01:23 +00:00
@ -162,3 +179,4 @@
currentUserMarks={currentUserMarks}
otherUserMarks={otherUserMarks}
/>
{noFiles ? (
Owner

If you keep the noFiles flag, the following two evaluations should be refactored using the ternary operator.

If you keep the `noFiles` flag, the following two evaluations should be refactored using the ternary operator.
m marked this conversation as resolved
enes reviewed 2025-01-14 09:47:42 +00:00
@ -45,0 +44,4 @@
`${file.name} uploaded to following file storages: ${urls.join(', ')}`
)
// This bit was returning an url, and return of this function is being set to mark.value, so it kind of
// does not make sense to return an url to the file storage
Member

Original value holds coordinates that replicate the signature strokes and depending on the person's signature can be quite large.
Online flow - It's uploaded as a separate blossom file and returned as URL to be fetched when needed.
Offline - the actual value as is kept as is and not replaced by a URL since we don't care about size in this case.

Original value holds coordinates that replicate the signature strokes and depending on the person's signature can be quite large. Online flow - It's uploaded as a separate blossom file and returned as URL to be fetched when needed. Offline - the actual value as is kept as is and not replaced by a URL since we don't care about size in this case.
Author
Owner

Hm, I did not see how it is fetched when mark.value is a URL, can you point to a file where that happens?

Hm, I did not see how it is fetched when mark.value is a URL, can you point to a file where that happens?
Member

the MarkStrategy interface and fetchAndDecrypt and encryptAndUpload functions handle that

the `MarkStrategy` interface and `fetchAndDecrypt` and `encryptAndUpload` functions handle that
@ -168,6 +169,7 @@ export const SignPage = () => {
createSignatureContent.markConfig,
usersPubkey!
)
// TODO figure out why markConfig does not contain the usersPubkey when multiple signer
Member

Is this possibly a new issue/bug?

Is this possibly a new issue/bug?
Author
Owner

sorry, leftover todo, I introduced the issue while refactoring, and also fixed it

sorry, leftover todo, I introduced the issue while refactoring, and also fixed it
m marked this conversation as resolved
@ -490,6 +520,189 @@ export const getUserAppDataFromBlossom = async (
return parsedContent
}
// /**
Member

Is this left from the NDK merge (the whole commented block)?

Is this left from the NDK merge (the whole commented block)?
m marked this conversation as resolved
eugene reviewed 2025-01-14 10:00:57 +00:00
@ -94,2 +97,3 @@
const ndkRelayList = await getNDKRelayList(pubkey)
const ndkRelayListPromise = getNDKRelayList(pubkey)
const serverMapPromise = getFileServerMap(pubkey, fetchEvent)
Owner

What is the advantage of using a map over list/array?

In my view, a map is useful when it is possible to query items by key, which is not something we will do with blossom drives.

An array seems a more straightforward choice. You won't need to change an object into an array further down the line when you intract with it.

What is the advantage of using a map over list/array? In my view, a map is useful when it is possible to query items by key, which is not something we will do with blossom drives. An array seems a more straightforward choice. You won't need to change an object into an array further down the line when you intract with it.
Author
Owner

We were discussing that in future, we will have read and write servers, so I prepared for it:

image.png

We were discussing that in future, we will have read and write servers, so I prepared for it: ![image.png](/attachments/a141c540-0ab3-4593-af5d-4d7afb4bb348)
9.6 KiB
Owner

let's put this explanation in a code comment

let's put this explanation in a code comment
Owner

You could have an entity FileServer:

{ url: string, read: boolean, write: boolean }

servers would hold an array of FileServer objects.

Again, in my view, that is a preferred approach, because we have no ability (or reason, really) to query file servers by key. And in the majority of cases that I've seen, the fileServerMap needs to be transformed into an array of keys, values, or entries, in order to interact with it.

You could have an entity FileServer: `{ url: string, read: boolean, write: boolean } ` `servers` would hold an array of FileServer objects. Again, in my view, that is a preferred approach, because we have no ability (or reason, really) to query file servers by key. And in the majority of cases that I've seen, the fileServerMap needs to be transformed into an array of keys, values, or entries, in order to interact with it.
eugene reviewed 2025-01-14 10:01:45 +00:00
@ -96,0 +99,4 @@
const serverMapPromise = getFileServerMap(pubkey, fetchEvent)
const [ndkRelayList, serverMap] = await Promise.all([
ndkRelayListPromise,
Owner

Instead of storing promises in a separate variable, you can pass in the functions that return promises directly in Promise.all.

Instead of storing promises in a separate variable, you can pass in the functions that return promises directly in `Promise.all`.
m marked this conversation as resolved
eugene reviewed 2025-01-14 10:02:25 +00:00
@ -100,3 +110,4 @@
return appPrivateRoutes.relays
}
if (Object.keys(serverMap).length < 1) {
Owner

This transformation can be avoided if you change serverMap to be an array.

This transformation can be avoided if you change `serverMap` to be an array.
Author
Owner
https://git.nostrdev.com/sigit/sigit.io/pulls/278#issuecomment-3772
eugene reviewed 2025-01-14 10:06:35 +00:00
@ -79,2 +79,3 @@
* We keep the last 10 versions
*/
blossomUrls: string[]
blossomVersions: BlossomVersion[]
Owner

Why not keep the existing property, blossomUrls? It looks like the contect is exactly the same, an array of strings.

Ah, I get it - we now have an archive of blossom urls.

~~Why not keep the existing property, `blossomUrls`? It looks like the contect is exactly the same, an array of strings.~~ Ah, I get it - we now have an archive of blossom urls.
m marked this conversation as resolved
eugene reviewed 2025-01-14 10:14:46 +00:00
@ -244,3 +255,2 @@
// Insert new blossom URL at the start of the array
blossomUrls.unshift(newBlossomUrl)
// insert new server (blossom) urls at the start of the array
Owner

Should it be added at the end of the array instead? It would make sense, going from the first version to the last version, i.e. 1, 2 .... n

Should it be added at the end of the array instead? It would make sense, going from the first version to the last version, i.e. 1, 2 .... n
Author
Owner

This is how it was done so far, the latest version was on top I don't know the reasoning

This is how it was done so far, the latest version was on top I don't know the reasoning
m marked this conversation as resolved
eugene reviewed 2025-01-14 10:21:17 +00:00
@ -0,0 +50,4 @@
const fetchFileServers = async () => {
if (usersPubkey) {
await getFileServerMap(usersPubkey, fetchEvent).then((res) => {
Owner

You should use either the async await syntax or promise chaining, not both of them at the same time.

It's probably best to refactor to: const servers = await getFileServerMap(usersPubkey, fetch event)

You should use either the async await syntax or promise chaining, not both of them at the same time. It's probably best to refactor to: `const servers = await getFileServerMap(usersPubkey, fetch event)`
Author
Owner

ah very strange, it was on accident

ah very strange, it was on accident
m marked this conversation as resolved
eugene reviewed 2025-01-14 10:31:12 +00:00
@ -0,0 +167,4 @@
if (res && res.data?.toLowerCase().includes('blossom server')) {
resolve(true)
} else {
reject(false)
Owner

I don't think you need to reject the promise in this scenario. You can return a resolved promise with either true (if you received the expected response) or false, and you won't need to do rely on catching the error in the client code to make a business logic decision.

I don't think you need to reject the promise in this scenario. You can return a resolved promise with either `true` (if you received the expected response) or `false`, and you won't need to do rely on catching the error in the client code to make a business logic decision.
Author
Owner

It makes very much sense, another "will fix it later" which never happened...

It makes very much sense, another "will fix it later" which never happened...
m marked this conversation as resolved
eugene reviewed 2025-01-14 10:33:17 +00:00
@ -0,0 +109,4 @@
setBlossomServersMap(tempBlossomServersMap)
setNewServerURL('')
publishFileServersList(tempBlossomServersMap)
Owner

Similar to my point elsewhere in the PR, I don't think we should be storing blossoms in a map. It should be an array instead.

Similar to my point elsewhere in the PR, I don't think we should be storing blossoms in a map. It should be an array instead.
Author
Owner
https://git.nostrdev.com/sigit/sigit.io/pulls/278#issuecomment-3772
eugene reviewed 2025-01-14 11:54:25 +00:00
@ -168,6 +169,7 @@ export const SignPage = () => {
createSignatureContent.markConfig,
usersPubkey!
)
Owner

Sounds like a pretty critical issue. Can you please add more information and create an issue?

Sounds like a pretty critical issue. Can you please add more information and create an issue?
Author
Owner

I am not sure what you mean, can you check if you maybe missed the place in the code?

I am not sure what you mean, can you check if you maybe missed the place in the code?
eugene reviewed 2025-01-14 12:02:05 +00:00
@ -298,0 +298,4 @@
return
}
for (let i = 0; i < zipUrls.length; i++) {
Owner

Does it mean that if a user has 3 blossom servrs, there will be 2 requests?

Of these two, the second request will over-write the result of the first one?

If you do need to iterate, you can probably use .map to handle promises and then set the value once they have been awaited, to make the code more declarative.

Does it mean that if a user has 3 blossom servrs, there will be 2 requests? Of these two, the second request will over-write the result of the first one? If you do need to iterate, you can probably use `.map` to handle promises and then set the value once they have been awaited, to make the code more declarative.
Author
Owner

It will take the first one which is valid, and it will only send more requests if the hash of the first one fails or so, I did iteration to be sequential on purpose, for that reason

It will take the first one which is valid, and it will only send more requests if the hash of the first one fails or so, I did iteration to be sequential on purpose, for that reason
Owner

rather than comment here, can we provide a comment in the code?

rather than comment here, can we provide a comment in the code?
Owner

Ok, thanks.

What is the reason to add break at the end of the last loop? Isn't the loop going to finish naturally because you increment i?

A bit of a nitpick, but I would also suggest using the for ... of loop over the 'vanilla' JS loop, because it is a little bit more declarative and still gives you the same sequential processing option and breaking out of the loop once you get the result.

Ok, thanks. What is the reason to add `break` at the end of the last loop? Isn't the loop going to finish naturally because you increment `i`? A bit of a nitpick, but I would also suggest using the `for ... of` loop over the 'vanilla' JS loop, because it is a little bit more declarative and still gives you the same sequential processing option and breaking out of the loop once you get the result.
eugene reviewed 2025-01-14 12:08:49 +00:00
@ -63,6 +63,7 @@ import { MarkRender } from '../../components/MarkTypeStrategy/MarkRender.tsx'
interface PdfViewProps {
files: CurrentUserFile[]
noFiles?: boolean
Owner

The same question as before - do we need noFiles, and if yes, can you some comment as to why.

The same question as before - do we need `noFiles`, and if yes, can you some comment as to why.
m marked this conversation as resolved
eugene reviewed 2025-01-14 12:14:06 +00:00
@ -432,2 +460,3 @@
if (!zip) return
if (!zip) {
if (!isLastZipUrl) continue // Skip to next zipUrl
Owner

Can you specify why it needs to break out of the loop if it's the last zip url?

Can you specify why it needs to break out of the loop if it's the last zip url?
Author
Owner

I added a comment

I added a comment
m marked this conversation as resolved
eugene reviewed 2025-01-14 12:16:29 +00:00
@ -455,3 +484,2 @@
const hash = await getHash(arrayBuffer)
const hash = await getHash(entryArrayBuffer)
if (hash) {
Owner

You don't need this if statement. getHash returns a string or null, so you can directly assign hash instead of re-assigning null.

You don't need this if statement. `getHash` returns a string or null, so you can directly assign `hash` instead of re-assigning `null`.
m marked this conversation as resolved
eugene reviewed 2025-01-14 12:22:16 +00:00
@ -0,0 +2,4 @@
import { RestoreState } from '../actions'
import { ServerMap } from '../../types'
export type ServersState = {
Owner

I think Servers is too ambiguous to use as a type. It should either be FileServer or BlossomServer. FileServer is a better option if we want to allow for a possibility that in the future there will be other compatible standards other than Blossom.

I think `Servers` is too ambiguous to use as a type. It should either be `FileServer` or `BlossomServer`. `FileServer` is a better option if we want to allow for a possibility that in the future there will be other compatible standards other than Blossom.
m marked this conversation as resolved
eugene reviewed 2025-01-14 12:27:04 +00:00
@ -22,6 +27,8 @@ export const DEFAULT_LOOK_UP_RELAY_LIST = [
'wss://purplepag.es'
]
export const MAXIMUM_BLOSSOMS_LENGTH = 3
Owner

Should it be configurable as well?

Should it be configurable as well?
Author
Owner

For now I don't think so, what do @b thinks?

For now I don't think so, what do @b thinks?
Owner

it can be a seperate issue / PR if we decide to do that (probably we'd combine it with read-only servers)

it can be a seperate issue / PR if we decide to do that (probably we'd combine it with read-only servers)
m marked this conversation as resolved
eugene reviewed 2025-01-14 12:28:52 +00:00
@ -0,0 +27,4 @@
authors: [npub]
}
const event = await fetchEvent(eventFilter).catch((err) => {
Owner

It would be better to wrap the whole body of the function in the try catch block, rather than just catch an error in this one instance.

It would be better to wrap the whole body of the function in the `try catch` block, rather than just catch an error in this one instance.
m marked this conversation as resolved
eugene reviewed 2025-01-14 12:34:32 +00:00
@ -411,2 +416,2 @@
// Return the URL of the uploaded file
return response.data.url as string
// Upload the file to the file storage services using Axios
for (const preferredServer of preferredServers) {
Owner

You could use .map to collect promises in Promise.all to make the code more declarative.

You could use `.map` to collect promises in `Promise.all` to make the code more declarative.
m marked this conversation as resolved
m added 1 commit 2025-01-15 09:38:46 +00:00
Merge branch 'staging' into issue-274
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 40s
f9fc368900
m added 2 commits 2025-01-15 09:39:31 +00:00
chore(git): pull merge
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 46s
19b9374ca0
m added 1 commit 2025-01-15 09:41:40 +00:00
chore: comment
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 44s
28150c424e
m added 1 commit 2025-01-15 09:45:52 +00:00
chore: comment
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 38s
5188f27624
m added 1 commit 2025-01-15 09:47:29 +00:00
fix: shortened imports
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 44s
34b993db20
All checks were successful
Open PR on Staging / audit_and_check (pull_request) Successful in 44s
This pull request has changes conflicting with the target branch.
  • src/components/MarkTypeStrategy/Signature/index.tsx
  • src/components/PDFView/PdfMarking.tsx
  • src/hooks/useSigitMeta.tsx
  • src/pages/create/index.tsx
  • src/pages/settings/relays/index.tsx
  • src/pages/sign/index.tsx

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue-274:issue-274
git checkout issue-274
Sign in to join this conversation.
No description provided.