-
Notifications
You must be signed in to change notification settings - Fork 8
Public graph integration #154
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
6a07e3f
to
9034a6d
Compare
9034a6d
to
60fe33e
Compare
28afc98
to
d36ef93
Compare
d36ef93
to
0ded21d
Compare
1c4b753
to
e5f6b80
Compare
e5f6b80
to
5283c61
Compare
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.
Pull Request Overview
This PR integrates public graph functionality by introducing smart account wallet client support and updating various entity query and mutation components for public, local, and merged operations. In addition, filter types for entity fields and a new properties/types creation component have been added.
- Added support for smart account wallet client integration in public graph operations.
- Updated and added new components for user and todo management across different modes (public, local, merged, read-only, filter).
- Introduced new type definitions for filters and a component to create properties and types.
Reviewed Changes
Copilot reviewed 82 out of 83 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
apps/events/src/lib/smart-account.ts | Added smart account wallet client integration. |
apps/events/src/filter-types.ts | Introduced type definitions for various filter options. |
apps/events/src/components/users/*.tsx | Updated user management components across different modes. |
apps/events/src/components/todos*.tsx | Integrated public/local todo operations and added new creation/publishing logic. |
apps/events/src/components/ui/modal.ts | Added modal component with keyboard support. |
apps/events/src/components/create-properties-and-types.tsx | Added a component to create graph properties and types. |
Files not reviewed (1)
- apps/events/package.json: Language not supported
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.
LGTM!
A few comments but nothing that can't be addressed in separate issues/PRs imo.
I did a very quick pass, so take the review with a grain of salt... a lot of the schema things would take me a lot longer to understand (for instance, no idea what the VariantSchema is).
@@ -0,0 +1,14 @@ | |||
import { getSmartAccountWalletClient as grc20getSmartAccountWalletClient } from '@graphprotocol/grc-20'; |
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.
it would be good to expose this and the other pieces from grc-20 from hypergraph so that users don't need to have grc-20 in their apps... but I guess we can solve this together with the new auth flow
preparePublish: preparePublishUsers, | ||
} = useQuery(User); | ||
const space = useHypergraphSpace(); | ||
const createTodo = useCreateEntity(Todo2); |
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.
I wonder if it might be nicer to have all these entity actions in a single hook, e.g. const { createEntity: createTodo, updateEntity: updateTodo, deleteEntity: deleteTodo } = useEntityActions(Todo2)
?
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.
interesting suggestion, I have no strong opinion here. I'd say let's bring it up in the sync call :)
let spaceId = Utils.generateId(); | ||
|
||
try { | ||
if (smartAccountWalletClient?.account) { |
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.
Shouldn't public space creation be optional, or are we expecting that private spaces will always have a matching public space? Maybe createSpace could take an options object where we can specify whether it's a public, private or mixed space?
(I imagine there will be a lot of cases where a space is only used for private data sync, and creating a space in the KG would be a waste of resources)
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.
Also, if we're creating private/public spaces with the same ids, I suppose we should also make invitations work for both the public and private space?
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.
|
||
const DEFAULT_RPC_URL = 'https://rpc-geo-genesis-h0q2s21xx8.t.conduit.xyz'; | ||
|
||
const GEOGENESIS: Chain = { |
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.
Let's have the option, when creating the wallet client, to choose Geo testnet instead?
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.
yeah, if we keep testnet around then it makes sense. Byron mentioned he was about to completely shut it down
import { md5 } from '@noble/hashes/legacy'; | ||
import { v4 } from 'uuid'; | ||
|
||
function createIdFromUniqueString(text: string) { |
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.
Seems like this file, and a lot of the things in this src/internal folder, could eventually move to the core hypergraph package, right?
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.
spot on, will do this!
} | ||
|
||
const cid = await Ipfs.publishEdit({ | ||
name: 'Update todos', |
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.
I imagine this can be a parameter of the function call, or we could use a more generic string?
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.
good catch, added an issue #172
7b2d9b1
to
5910d3b
Compare
No description provided.