Skip to content

Support verify data in x mb#37

Merged
GAllen98 merged 2 commits intomainfrom
support-verify-data-in-x-mb
Feb 5, 2025
Merged

Support verify data in x mb#37
GAllen98 merged 2 commits intomainfrom
support-verify-data-in-x-mb

Conversation

@GAllen98
Copy link
Contributor

  • A new type has been created to represent the data in x-mb from the manifest.
  • Verify data can now be added to x-mb instead of being passed through flags on the command line

@GAllen98 GAllen98 changed the base branch from main to merge-register-deploy-and-update January 20, 2025 13:04
Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a lot of new stuff and no tests. I think this could be improved in a few different ways.

@SurgeCode
Copy link
Member

If the new verify content is in the spec are we ingesting it on the wallet side as well? @GAllen98

@GAllen98 GAllen98 force-pushed the support-verify-data-in-x-mb branch from dc92dce to 0e8709c Compare January 23, 2025 15:55
Base automatically changed from merge-register-deploy-and-update to main January 24, 2025 15:48
Copy link
Contributor

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@GAllen98 GAllen98 force-pushed the support-verify-data-in-x-mb branch from 0e8709c to f8e3aea Compare January 27, 2025 13:43
chainIds?: number[];
};

export function validateXMbSpec(xMbSpec: unknown): asserts xMbSpec is XMbSpec {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: type guards usually follow the pattern

Example: https://github.com/BitteProtocol/near-safe/blob/6637da030c3f022029618c4993ec21be1cbaf655/src/types/guards.ts#L6

Suggested change
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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, type guards usually return true or false... in this case we return undefined when it passes all the assertions...

@bh2smith
Copy link
Contributor

bh2smith commented Jan 27, 2025

I added some tests on the validation as a PR based on this one.

May be a good starting point:

#42

Try it out with bun test.

In particular, the test demonstrates that the following object would pass validation:

{
    "account-id": "name",
    assistant: {
      name: "assistantName",
      description: "assistantDescription",
      instructions: "assistantInstructions",
  },
}

@GAllen98 GAllen98 force-pushed the support-verify-data-in-x-mb branch from acc5be1 to a070d09 Compare February 5, 2025 11:52
- 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
@GAllen98 GAllen98 force-pushed the support-verify-data-in-x-mb branch from a070d09 to 688d9ac Compare February 5, 2025 13:57
@GAllen98 GAllen98 merged commit a6772b8 into main Feb 5, 2025
2 checks passed
@GAllen98 GAllen98 deleted the support-verify-data-in-x-mb branch February 5, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants