-
Notifications
You must be signed in to change notification settings - Fork 8
chore: sign update messages before sending #135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f4a87f5
to
e1e321c
Compare
Still in progress.
|
ae96400
to
25b94a3
Compare
Still need to see if it works, but I think it's ready for review at this point |
apps/server/src/index.ts
Outdated
@@ -1,5 +1,6 @@ | |||
import { parse } from 'node:url'; | |||
import { Identity, Messages, SpaceEvents, Utils } from '@graphprotocol/hypergraph'; | |||
import { recoverUpdateMessageSigner } from '@graphprotocol/hypergraph/messages/signed-update-message'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you fix the import? afaik we should export it properly and not do deep imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, missed this, will fix
}); | ||
|
||
space?.automergeDocHandle?.update((existingDoc) => { | ||
const [newDoc] = automerge.applyChanges(existingDoc, automergeUpdates); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we gather all the automergeUpdates and then apply them. I actually didn't do it properly in the other case, but this should be the way to go. Can you update the function to do it this way?
The reason is that afaik this is much more performant than applying ever update in a for loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, will fix
Tested running the events app locally and can confirm updates are propagated correctly afaict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work 👏
Closes #70
WIP