-
Notifications
You must be signed in to change notification settings - Fork 202
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: v2 event decoding #415
Conversation
678cd09
to
f3fb706
Compare
5c588e5
to
f1e27de
Compare
f1e27de
to
5c588e5
Compare
b8fb384
to
7863484
Compare
7863484
to
8b8b633
Compare
580373b
to
e6491e8
Compare
df2fafb
to
0afa82d
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.
Nothing blocking (except from the comment in package.json of world/store). Are you planning to add all TODO
comments to some tracker and link the tracker in the code?
Almost there!!
@@ -7,7 +7,7 @@ export async function loadStoreConfig(configPath?: string) { | |||
const config = await loadConfig(configPath); | |||
|
|||
try { | |||
return parseStoreConfig(config); | |||
return parseStoreConfig(config as any); |
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 is this needed?
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.
import { decodeStaticField } from "./decodeStaticField"; | ||
import { decodeDynamicField } from "./decodeDynamicField"; | ||
|
||
export const decodeData = (schema: TableSchema, hexData: string): Record<number, any> => { |
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 think the function
definitions are a bit more common in the current codebase, we should align everything at some point for consistency. Not blocking for this PR though.
console.log("fetching metadata for table", { table: table.toString(), world: world.address }); | ||
const metadataPromise = Promise.all([ | ||
registerSchema(world, metadataTableId), | ||
// TODO: figure out how to pass in rawSchema, it was giving me "incorrect length" errors before |
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.
maybe we have to pad the raw schema string to 32 bytes?
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.
not sure, but also not too worried since this is a getter and we still have to do both fetch calls anyway
// TODO: figure out how to pass in rawSchema, it was giving me "incorrect length" errors before | ||
world["getRecord(uint256,bytes32[])"](metadataTableId.toHexString(), [table.toHexString()]), | ||
]).then(([metadataSchema, metadataRecord]) => { | ||
// TODO: error if schema or record not found? |
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.
should we at least log a warning here?
packages/store/package.json
Outdated
@@ -13,7 +13,7 @@ | |||
"prepare": "yarn build && chmod u+x git-install.sh", | |||
"git:install": "bash git-install.sh", | |||
"codegen": "ts-node ./scripts/codegen.ts", | |||
"tablegen": "../cli/dist/mud.js tablegen", | |||
"tablegen": "mud tablegen", |
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'd prefer keeping the local version for this script, so it works in the CI when we do changes to tablegen (without releasing a new version)
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.
what if, instead, we have CI use a locally linked mud CLI?
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.
That would work too, we just have to set it up. If it "just works" I'm down to do that instead in this PR, but wouldn't lose too much time on it, we can always change it in a follow up
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.
will change it back for ease and add an issue to come back to this
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.
packages/world/package.json
Outdated
@@ -12,7 +12,7 @@ | |||
"scripts": { | |||
"prepare": "yarn build && chmod u+x git-install.sh", | |||
"git:install": "bash git-install.sh", | |||
"tablegen": "../cli/dist/mud.js tablegen", | |||
"tablegen": "mud tablegen", |
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.
Same here, see comment above (in store/package.json)
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.
@@ -19,7 +19,7 @@ export const ecsEventFromLog = async ( | |||
const tableId = TableId.fromBytes32(utils.arrayify(args.table)); | |||
const component = tableId.toString(); | |||
// TODO: revisit key tuple encoding for client | |||
const entity = args.key.join(":") as EntityID; | |||
const entity = `v2:${args.key.join(":")}` as EntityID; |
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 might break something if I want to use v2 with ECS, and want to refer to another entity in my component
Example: OwnedBy
component, Bob (0x01) is owned by Alice (0x02), so I have an entry in the OwnedBy table with key 0x01
and value 0x02
. Now I want to do an recs query on the client to get all entities owned by Alice, so I do getEntitiesWithValue(OwnedBy, {value: Alice})
, but because Alice is normalized to v2:0x02
it wouldn't find the 0x02
value of key 0x01
.
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.
updated in 9c13cd7
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 go!
Parses v2 events into schema registrations and data to populate client ECS with it.
TODO
mud.config
importsparseConfig
more strongly typed and carry through statically defined valuesStoreConfig
andStoreUserConfig
, etc. so we default to user-first