-
Notifications
You must be signed in to change notification settings - Fork 74
Typescript improvements and Address Checksumming #65
Conversation
… easy imports, added prepare for dist generation upon install
package.json
Outdated
@@ -13,7 +13,8 @@ | |||
"scripts": { | |||
"build": "rimraf dist/* && tsc --emitDeclarationOnly && cross-env NODE_ENV=production webpack --config webpack.config.js", | |||
"test": "jest -c jest.config.js", | |||
"publish": "yarn build && yarn publish" | |||
"publish": "yarn build && yarn publish", | |||
"prepare": "npm run build" |
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.
What is the purpose of prepare?
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.
If the package is used as a dependency in a project, we should be able to use the types provided in the dist folder.
"prepare" tells the npm installer to run a build once the package is installed, in order to generate the dist directory.
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.
@mickys I think the typings are already included in the dist
folder as part of the NPM distribution. It seems I forgot to add the types
field. Can you add it in? If you do, I don't think prepare
is necessary.
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.
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.
There is a "typings": "dist/index.d.ts" in the package.json file, which is synonymous to types. So if it was supposed to work it should have.
( unless yarn publish / npm.org does this, which i'm unable to test )
There are 3 ways these can be provided to the user as far as i'm aware
-
Provide said typings in the "dist" folder or another one "lib" maybe.
this does create the dependency of building before committing -
Create a typings module and PR it to @types ( https://github.com/DefinitelyTyped/DefinitelyTyped ) to be included when needed.
Does take some time to get accepted thou and afaik these are meant for modules that don't do it themselves -
Use "prepare" to generate dist or lib folder.
I'm fine with whichever option you guys prefer, thou
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.
@mickys you're right. It looks like there has been some mistake when the build was published. We'll rectify this issue. prepare
should not be necessary.
@edisonljh which folder do we normally publish?
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.
I've removed the prepare instruction from the PR.
Feel free to merge then do the changes required.
Thanks @mickys. This looks good. Would you be able to add a few more test vectors for the checksum tests? You can do something like |
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.
More test vectors to be added.
Added more checksum test vectors, see checksum.fixtures.ts |
I've removed the prepare instruction from the PR. |
This looks good! Thanks, this is a good PR! 👍 |
Thanks @mickys ! |
Description
1. Cleanup and Typescript improvements.
1.1. New usage example no longer requires "Zilliqa" class instantiation to get to the util library
2. Address Checksumming based on EIP-55
see: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-55.md
2.1. Methods:
methods support both standard and 0x prepended addresses
2.2. Usage Example
methods support both standard and 0x prepended addresses as inputs
2.3. Tests
Added tests for these in tests/util.spec.ts
Review Suggestion
Checksumming uses: hashjs.sha256() since it was already imported in the util class
Status
Implementation