-
Notifications
You must be signed in to change notification settings - Fork 12
Better utilize naj 6 / nearjs 2
#20
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
Conversation
This commit fixes the action creation, now, instead of manually creating an action we rely on `near-api-js` to create it for us, which should make sure that now actions are compatible with all tools in the ecosystem Furthermore, I fixed a couple of small problems in the tests that and types that were hampering using the library locally for me
| "jest-environment-node": "^29.7.0", | ||
| "prettier": "^3.2.5", | ||
| "ts-jest": "^29.1.0", | ||
| "ts-node": "^10.9.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed this locally
|
|
||
| const hash = createHash('sha512').update(signingData).digest() | ||
| const signingHash = new Uint8Array(hash.slice(0, 32)) | ||
| const signingHash = new Uint8Array(hash.subarray(0, 32)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slice is deprecated
GregProuty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good, thanks for catching and fixing these issues. One concern, let’s avoid re-introducing near-api-js, we should stay standardized on the v2 @near-js/* packages to prevent mixing v1/v2 deps.
I ran into errors using near-api-js with ed25519 transactions in the Near adapter.
near-api-js v5 pulls in @near-js/* v1 as dependencies. This creates runtime conflicts where two different Borsh schemas try to serialize the same transactions, causing "Enum key not found" errors and breaking Ed25519 signing in the NEAR adapter.
We should be able to replace the near-api-js imports with v2 equivalents:
// Change these imports:
import { providers } from 'near-api-js'
import { getTransactionLastResult } from 'near-api-js/lib/providers'
// To these:
import { JsonRpcProvider } from '@near-js/providers'
import { getTransactionLastResult } from '@near-js/utils'
| "import-local": "^3.2.0", | ||
| "jest": "^29.7.0", | ||
| "jest-environment-node": "^29.7.0", | ||
| "near-api-js": "^6.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as devdependency for the tests that use it, ideally we remove it entirely in the future
naj 6 / nearjs 2
naj 6 / nearjs 2naj 6 / nearjs 2
GregProuty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just one nit, ChainSignatureContract.ts line 64: I'm getting a typescript error building the library src/contracts/ChainSignatureContract.ts(64,7): error TS2345: Argument of type 'JsonRpcProvider[]' is not assignable to parameter of type 'Provider[]'
should be easy to fix with a type assertion.
Other than that looks good to merge
This PR updates several files on the repo in order to better use new functionality from
near-api-js(specifically around theAccountclass)signAndSendTransactionssignfunction to usenear-api-jsActions, which are now both supported by NAJAccountsand Wallet SelectorWalletsI further needed to make other changes:
typedocon PRs (will still run after a merge)@contract/account getNearAccountfunction to an util, as it was only used internally and for testingts-nodeas I needed it locally to run testsslicefunction bysubarrayas otherwise the tests were failingsendTransactionUntil, it was no longer necessary asnajAccountsnow implement awaitUntil?parameter when sending transactions