-
Notifications
You must be signed in to change notification settings - Fork 189
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
feat(cli,recs,std-client): update RECS components with v2 key/value schemas #1195
Conversation
🦋 Changeset detectedLatest commit: b3c16ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
3f8a77a
to
909737f
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.
Lgtm, just small nits/questions. Is this a breaking change in any way?
@@ -27,8 +29,24 @@ export function getRecsV1TableOptions(config: StoreConfig): RecsV1TableOptions { | |||
name: tableData.name, | |||
}; | |||
|
|||
// TODO: move user type -> abi type into our config expanding step rather than sprinkled everywhere |
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 create an issue for this to keep track of the TODO
id: ${JSON.stringify(tableId.toHex())}, | ||
metadata: { | ||
componentName: ${JSON.stringify(name)}, | ||
tableName: ${JSON.stringify([namespace, name].join(":"))}, |
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.
why do we need to run JSON.stringify
on these? Aren't they already strings?
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.
this is generating JS code and namespace/name are technically user generated, so it's a bit safer to encode to string with JSON.stringify
rather than wrapping it in quotes
} else { | ||
mappings[keccak256(contractId)] = key; | ||
mappings[ | ||
keccak256(typeof component.metadata?.contractId === "string" ? component.metadata.contractId : component.id) |
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.
would this be equivalent?
keccak256(typeof component.metadata?.contractId === "string" ? component.metadata.contractId : component.id) | |
keccak256(component.metadata?.contractId ?? component.id) |
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 had this before, but TS complained about the contractId
value being {}
or something? Might be related to Metadata
default being Record<string, unknown>
@@ -26,7 +26,7 @@ export type DecodedSystemCall< | |||
updates: DecodedNetworkComponentUpdate[]; | |||
}; | |||
|
|||
export type ContractComponent = Component<Schema, { contractId: string; tableId?: string }>; | |||
export type ContractComponent = Component<Schema>; |
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.
does this break any assumptions of existing code? maybe the ecs browser, i believe it's still in use by sky strife? (cc @Kooshaba)
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.
ECS browser got inlined into sky strife and I fixed a couple of those things in my PR over there: https://github.com/latticexyz/skystrife/pull/167
I don't think so? If you're using plain RECS, this shouldn't have any impact. You'll just need to bump RECS + CLI versions together and rebuild component definitions. It was only Sky Strife (they overloaded |
and move custom RECS schema type default from
undefined
tounknown
pulled out of #1113