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.

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

ABI generation #301

Merged
merged 15 commits into from
Dec 4, 2022
Merged

ABI generation #301

merged 15 commits into from
Dec 4, 2022

Conversation

itegulov
Copy link
Contributor

This is still pretty raw, but sharing for visibility.

Instructions for running:

  1. Clone https://github.com/near/near-abi-js to a sibling directory and run yarn install && yarn build (needed because it is not released yet)
  2. Clone https://github.com/itegulov/typescript-json-schema to a sibling directory and run yarn install && yarn build (needed because I had to expose some API that makes life much much easier).
  3. Run yarn build:counter-ts in examples and see what happens 👀

src/cli/abi.ts Outdated
Comment on lines 57 to 69
const diagnostics = ts.getPreEmitDiagnostics(program);
if (diagnostics.length > 0) {
diagnostics.forEach((diagnostic) => {
const message = ts.flattenDiagnosticMessageText(diagnostic.messageText, "\n");
if (diagnostic.file) {
const { line, character } = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start!);
console.error(`${diagnostic.file.fileName} (${line + 1},${character + 1}): ${message}`);
} else {
console.error(message);
}
});
return undefined;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can get rid of the preceding CLI step that does typechecking since this piece does effectively the same without having to instantiate tsc twice. But up to you guys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is still unclear to me if IBI is a separate package. If it is not - then we can do a TS check once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IBI?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, "ABI"

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/version.ts Outdated Show resolved Hide resolved
src/cli/abi.ts Outdated
Comment on lines 57 to 69
const diagnostics = ts.getPreEmitDiagnostics(program);
if (diagnostics.length > 0) {
diagnostics.forEach((diagnostic) => {
const message = ts.flattenDiagnosticMessageText(diagnostic.messageText, "\n");
if (diagnostic.file) {
const { line, character } = diagnostic.file.getLineAndCharacterOfPosition(diagnostic.start!);
console.error(`${diagnostic.file.fileName} (${line + 1},${character + 1}): ${message}`);
} else {
console.error(message);
}
});
return undefined;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is still unclear to me if IBI is a separate package. If it is not - then we can do a TS check once.

src/cli/abi.ts Show resolved Hide resolved
src/cli/abi.ts Outdated
target: ts.ScriptTarget.ES5,
module: ts.ModuleKind.CommonJS,
allowUnusedLabels: true,
};
Copy link
Member

Choose a reason for hiding this comment

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

So these overrides options load in basePath/tsconfig.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried to use contract's tsconfig.json, but then it picks up all TS files in this project, not just the one you directly specify (e.g. all examples under /examples, not just counter-ts). I will play around with this a bit more and see what can be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I figured out the right way to deal with this, PTAL.

While we are at this, how do you think the path to package.json and tsconfig.json should be specified? I have just added a 3rd and 4th argument to the CLI command, but that's probably not what we want.

src/cli/abi.ts Outdated Show resolved Hide resolved
@ailisp
Copy link
Member

ailisp commented Nov 16, 2022

Generally your implementation and comprehensive validation looks great!

src/cli/abi.ts Show resolved Hide resolved
@itegulov
Copy link
Contributor Author

I am working on stabilizing this and I have a couple of things that I am not sure how to go about.

  1. Should this be an opt-in feature for now? How do you guys imagine the CLI interface for ABI generation should look like? Do we also provide a separate command abi that just generates ABI without building the contract? I can match what we have in Rust, but not sure if that makes sense as the build tools are quite different, so your call.
  2. I have released https://github.com/itegulov/typescript-json-schema as near-typescript-json-schema for now just to unblock this PR, but not sure how you would like to handle this fork. We have a similar situation with https://github.com/near/schemafy/, so we just moved the fork under near org and are maintaining it ourselves. Ideally we should discuss this with the original repo maintainers and see if they would be open to exposing the APIs that we need, but in my experience open-source work is pretty slow, so I wouldn't count on it short-term (we are still waiting for schemafy maintainers' response for instance).

So far most examples work, only NFT is failing because bigint is apparently not supported by typescript-json-schema. There is this PR that was never merged though, so it seems like I can easily add support in our fork if you don't have any objections.

@volovyks
Copy link
Collaborator

@itegulov our build command now consists of several steps. I think ABI can become one of them. It should be easy to have as a separate command as well. I do not think that we need to keep it as a separate project.

@itegulov itegulov marked this pull request as ready for review December 2, 2022 03:27
@@ -67,7 +67,7 @@ export class ParkingLot {
}

@view({})
getCarSpecs({ name }: { name: string }) {
getCarSpecs({ name }: { name: string }): CarSpecs {
Copy link
Member

Choose a reason for hiding this comment

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

will we get warnings (or configure tsconfig in a way) when method doesn't have return type?

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Nice work! It's very solid implementation! Also thank you for fixing a few signatures!

Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Great work @itegulov!
Merging now, but do you think it's possible to test ABI somehow?

@volovyks volovyks merged commit 635f7e6 into develop Dec 4, 2022
@ailisp ailisp mentioned this pull request Jan 17, 2023
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.

4 participants