Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(manifests): add json schema for abi #1034

Merged
merged 29 commits into from
Jul 23, 2022
Merged

feat(manifests): add json schema for abi #1034

merged 29 commits into from
Jul 23, 2022

Conversation

Niraj-Kamdar
Copy link
Contributor

No description provided.

@Niraj-Kamdar Niraj-Kamdar marked this pull request as draft July 7, 2022 10:44
@Niraj-Kamdar Niraj-Kamdar marked this pull request as ready for review July 14, 2022 23:56
@Niraj-Kamdar Niraj-Kamdar changed the title feat(abi-json-schema): first iteration feat(manifests): add json schema for abi Jul 14, 2022
cbrzn
cbrzn reviewed Jul 18, 2022
View reviewed changes
Copy link
Contributor

@cbrzn cbrzn left a comment

Choose a reason for hiding this comment

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

This is amazing Niraj. Thanks for pushing this 🙏

packages/js/manifests/wrap/src/formats/wrap.info/deserialize.ts Show resolved Hide resolved
packages/js/manifests/wrap/src/formats/wrap.info/index.ts Outdated Show resolved Hide resolved
packages/templates/wasm/assemblyscript/src/__tests__/e2e/integration.spec.ts Outdated Show resolved Hide resolved
dOrgJelli
dOrgJelli reviewed Jul 23, 2022
View reviewed changes
packages/cli/src/lib/Compiler.ts
@@ -369,7 +369,7 @@ export class Compiler {
});

const info: WrapManifest = {
abi: filteredAbi,
abi: (filteredAbi as unknown) as WrapAbi,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit sketchy, and points out that we have 2 ABI types defined in schema/parse/abi and manifest/wrap.info/abi.

I looked into using the manifest/wrap.info/abi inside of the schema pipeline directly, but it is a bit complicated... I think we should create a new PR after this is merged to consolidate these definitions and only use the manifest's ABI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a final validation pass below this to ensure we don't write invalid ABIs.

dOrgJelli
dOrgJelli previously approved these changes Jul 23, 2022
View reviewed changes
@dOrgJelli dOrgJelli merged commit 84ac338 into origin-dev Jul 23, 2022
@cbrzn
Copy link
Contributor

cbrzn commented Jul 23, 2022

I looked into using the manifest/wrap.info/abi inside of the schema pipeline directly, but it is a bit complicated... I think we should create a new PR after this is merged to consolidate these definitions and only use the manifest's ABI.

I will push this forward

@dOrgJelli dOrgJelli mentioned this pull request Jul 24, 2022
Merged
@dOrgJelli dOrgJelli deleted the feat/json-schema-abi branch April 10, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbrzn cbrzn cbrzn left review comments

@dOrgJelli dOrgJelli dOrgJelli approved these changes

@namesty namesty Awaiting requested review from namesty namesty is a code owner

@krisbitney krisbitney Awaiting requested review from krisbitney

@nerfZael nerfZael Awaiting requested review from nerfZael

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Niraj-Kamdar @cbrzn @dOrgJelli