Conversation
bh2smith
left a comment
There was a problem hiding this comment.
I see a lot of new stuff and no tests. I think this could be improved in a few different ways.
|
If the new verify content is in the spec are we ingesting it on the wallet side as well? @GAllen98 |
dc92dce to
0e8709c
Compare
There was a problem hiding this comment.
Love the type validation! Thanks for including it. Might seem like a bit much to ask, but I was wondering if we might wanna test some of this code with real example?
The reason for having test fixtures in places like this prevent future devs from haphazardly breaking interface requirements that external parties might rely on. In some sense this is like "versioning" our specification and being able to determine when our specification changes are backwards compatible or introduce the need for a major version update. In fact, for our own sanity, we may even want to consider including a version tag on the structure so it's easier to parse older verses newer specs.
0e8709c to
f8e3aea
Compare
src/config/types.ts
Outdated
| chainIds?: number[]; | ||
| }; | ||
|
|
||
| export function validateXMbSpec(xMbSpec: unknown): asserts xMbSpec is XMbSpec { |
There was a problem hiding this comment.
Nit: type guards usually follow the pattern
| export function validateXMbSpec(xMbSpec: unknown): asserts xMbSpec is XMbSpec { | |
| export function isXMbSpec(xMbSpec: unknown): asserts xMbSpec is XMbSpec { |
Might also use data for input var name.
There was a problem hiding this comment.
Also, type guards usually return true or false... in this case we return undefined when it passes all the assertions...
|
I added some tests on the validation as a PR based on this one. May be a good starting point: Try it out with In particular, the test demonstrates that the following object would pass validation: {
"account-id": "name",
assistant: {
name: "assistantName",
description: "assistantDescription",
instructions: "assistantInstructions",
},
} |
acc5be1 to
a070d09
Compare
- Update the readme to only have `deploy` from the register/deploy/update merge - Update the readme so the `verify` command describes the chainIds and categories flags
a070d09 to
688d9ac
Compare
x-mbfrom the manifest.x-mbinstead of being passed through flags on the command line