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

feat: add support for ABI #1045

Merged
merged 9 commits into from
Jan 25, 2023
Merged

feat: add support for ABI #1045

merged 9 commits into from
Jan 25, 2023

Conversation

itegulov
Copy link
Contributor

@itegulov itegulov commented Dec 19, 2022

Motivation

NEAR ABI is a language-agnostic format describing the interface of a contract. It enables developers not to have to manually specify the list of viewMethods and changeMethods. Additionally, it makes it possible to add caller-side type safety by validating arguments against their JSON Schema counterparts before making the call.

Description

This PR adds a new optional field abi that the user can pass when instantiating a Contract that is used to populate viewMethods/changeMethods with new entries. ajv is used as the JSON Schema validation engine due to its flexible support of various JSON Schema features.

Validation is still very much PoC and needs to be tested on a bigger set of samples.

Checklist

  • Read the contributing guidelines
  • Commit messages follow the conventional commits spec
  • Performed a self-review of the PR
  • Added automated tests
  • Manually tested the change

@itegulov itegulov requested a review from andy-haynes December 19, 2022 05:51
@changeset-bot
Copy link

changeset-bot bot commented Dec 19, 2022

🦋 Changeset detected

Latest commit: cc5e8a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
near-api-js Minor
@near-js/cookbook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -12,7 +12,7 @@
"alwaysStrict": true,
"outDir": "./lib",
"declaration": true,
"preserveSymlinks": true,
"preserveSymlinks": false,
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 have encountered a weird bug where adding ajv dependency messes up with pnpm's dependency model. It can't find some of the ajv transitive dependencies even though they are clearly there. I and @morgsmccauley have debugged the issue and found out that toggling this line fixes this.

Copy link
Contributor

@morgsmccauley morgsmccauley Dec 19, 2022

Choose a reason for hiding this comment

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

Adding more context while it's fresh:

This seems to be an issue within pnpm that has already been fixed: pnpm/pnpm#496, so not sure what seems to be the problem here.

pnpm will place all dependencies within their own node_modules folder and, if it is defined in package.json, symlink to that directory from the top level:

node_modules/
|  ajv ➛ node_modules/.pnpm/ajv@8.11.2/node_modules/ajv
|  .pnpm/
|  |  ajv@8.11.2/
│  |  |  node_modules/
|  │  |  |  ajv/
|  |  |  |  fast-deep-equal/ ➛ node_modules/.pnpm/fast-deep-equal@3.1.3/node_modules/fast-deep-equal
|  |  |  |  json-schema-traverse/ ➛ node_modules/.pnpm/json-schema-traverse@1.0.0/node_modules/json-schema-traverse
|  |  |  |  require-from-string/ ➛ node_modules/.pnpm/require-from-string@2.0.2/node_modules/require-from-string
|  |  |  |  uri-js/ ➛ node_modules/.pnpm/uri-js@4.4.1/node_modules/uri-js

This strategy makes all transitive dependencies inaccessible from your package code while still making them accessible to the dependency itself. In the above example, all of ajvs dependencies are next to it (in the bottom most directory), meaning ajv should be able to import/require it.

But when we use TS to build, we get the following error, which complains that ajv can't access uri-js. This doesn't make any sense because we can see it within node_modules/:

> near-api-js@1.1.0 compile /Users/morganmccauley/Developer/Repositories/near-api-js/packages/near-api-js
> tsc -p ./tsconfig.json

node_modules/ajv/dist/compile/resolve.d.ts(3,36): error TS2307: Cannot find module 'uri-js' or its corresponding type declarations.
node_modules/ajv/dist/types/index.d.ts(1,22): error TS2307: Cannot find module 'uri-js' or its corresponding type declarations.
node_modules/near-abi/lib/index.d.ts(1,29): error TS2307: Cannot find module 'json-schema' or its corresponding type declarations.

Setting preserveSymlinks to false is counter-intuitive here, but works.. When disabled, module resolution happens from where the symbolic link is defined, rather from where it actually resolves. I would think that we would want the latter, since that is where the transitive dependencies are defined, but this doesn't seem to be the case.

maybe we should raise this issue within pnpm itself 🤔

Copy link
Collaborator

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Awesome work @itegulov! I have some feedback around tests and how this extends the Contract interface but it's looking good.

Would you mind please also running npx changeset to register this as a minor change?

packages/near-api-js/test/contract_abi.test.js Outdated Show resolved Hide resolved
packages/near-api-js/src/contract.ts Outdated Show resolved Hide resolved
packages/near-api-js/src/contract.ts Show resolved Hide resolved
packages/near-api-js/src/contract.ts Outdated Show resolved Hide resolved
packages/near-api-js/src/contract.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Looks good, I just missed a couple things in validateArguments

packages/near-api-js/src/contract.ts Outdated Show resolved Hide resolved
packages/near-api-js/src/contract.ts Show resolved Hide resolved
@andy-haynes andy-haynes merged commit 9189e11 into master Jan 25, 2023
@andy-haynes andy-haynes deleted the daniyar/abi-support branch January 25, 2023 18:49
@github-actions github-actions bot mentioned this pull request Jan 25, 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.

3 participants