-
Notifications
You must be signed in to change notification settings - Fork 4
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: migrate to ts #1
Conversation
.vscode/settings.json
Outdated
@@ -0,0 +1,251 @@ | |||
{ | |||
"cSpell.words": [ | |||
"bitcoincash" |
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.
missing comma ,
at the end of line
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.
This is for the VSC extension so linting rules doesn't apply here
@@ -0,0 +1,251 @@ | |||
{ | |||
"cSpell.words": [ |
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.
Also I would suggest to sort these words alphabetically ;D
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.
Sorted
const regexp = new RegExp("^(ak_)([" + ALLOWED_CHARS + "]+)$"); // Begins with ak_ followed by | ||
|
||
const aeValidator: TChecksumValidator = { | ||
isValidAddress(address) { |
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.
It is on purpose to remove old args? I see that they weren't used but maybe users that use this package will pass all 3 args.
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.
The same also applies to methods for rest of crypto.
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.
These are additional args - unused
|
||
verifyChecksum: (address) => { | ||
const decodedBuffer = base58.decode(address); | ||
return decodedBuffer.length === 32 + 4; // add 4 because base is adding 4 characters, why is base 58 adding them? |
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 would suggest to leave a TODO comment ;D
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 was wodneriing if we should leave original code here but it lgtm.
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 changed it because npm package base58 decode function return type is different so that was cleanest&easiest refactor
* @param {String} address The target address | ||
* @param {Object} currency A currency from the ./currencies.js array | ||
* @param {String} networkType Network Type. Could be 'prod', 'both' and 'testnet' | ||
* @param {Array} addressFormats Array of formats. Options are: 'legacy', 'cashaddr', 'bitpay', 'slpaddr', 'all' |
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.
You have declared thisjust as string:
export type TAddress = string;
Can another crypto addresess have different address format?
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.
Btw. I see you have removed this comment for src/bitcoincash_validator.ts
, but left simialr comment in src/wallet_address_validator.ts
.
src/crypto/cnBase58.ts
Outdated
return b58; | ||
})(); | ||
|
||
export default cnBase58; |
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.
why cnBase58
not just base58
like before?
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.
cnBase58 is not Base58. Maybe types names are wrong 🤔
} | ||
} | ||
|
||
module.exports = methods |
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.
Do we have ts version for sha3? I don't see.
- simple-import-sort | ||
- eslint-plugin-prettier | ||
|
||
rules: |
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 I fan of single quotes I would recommend you to use single quotes for imports, strings etc :D
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.
Also ramp-instant uses single quotes mostly
.eslintrc.yml
Outdated
ignorePatterns: | ||
- '*.d.ts' | ||
- 'dist' | ||
- "rollup.config.ts" |
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.
- "rollup.config.ts" | |
- 'rollup.config.ts' |
@@ -1,256 +1,11 @@ | |||
{ | |||
"name": "@swyftx/api-crypto-address-validator", | |||
"name": "@ramp/api-crypto-address-validator", |
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.
@macbem shouldn't it be @ramp-network/api-crypto-address-validator
or just @ramp-network/crypto-address-validator
?
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.
@ramp-network/crypto-address-validator
package.json
Outdated
"module": "dist/ramp-crypto-address-validator.es5.js", | ||
"typings": "dist/types/wallet_address_validator.d.ts", | ||
"version": "1.0.0", | ||
"author": "RampNetwork", |
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.
"author": "RampNetwork", | |
"author": "Ramp Network <[email protected]>", |
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.
Changed, I miss out this one 😨
} | ||
}; | ||
|
||
// TODO refactor |
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.
Is it done/resolved?
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.
Same reason as here
return BCH.isTestnetAddress(address); | ||
} | ||
|
||
// TODO: check if this is okay? Defaults to true if not prod or testnet? Probably 'both'? |
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.
so is it okay?
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.
That's not my comment, was already in the lib
return hex; | ||
}; | ||
|
||
// TODO refactor |
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.
is it refactored?
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 didn't want to make refactor in this PR, because finding a potential error will be much harder
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.
This is part of the output of yarn test
(when run on macOS):
$ rollup -c rollup.config.ts
src/wallet_address_validator.ts → dist/ramp-crypto-address-validator.umd.js, dist/ramp-crypto-address-validator.es5.js...
(!) Missing shims for Node.js built-ins
Creating a browser bundle that depends on "buffer", "stream" and "string_decoder". You might need to include https://github.com/snowpackjs/rollup-plugin-polyfill-node
(!) Plugin node-resolve: preferring built-in module 'buffer' over local alternative at '/Users/andrzejwodkiewicz/Ramp/crypto-address-validator/node_modules/buffer/index.js', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning
(!) Plugin node-resolve: preferring built-in module 'string_decoder' over local alternative at '/Users/andrzejwodkiewicz/Ramp/crypto-address-validator/node_modules/string_decoder/lib/string_decoder.js', pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning
(!) Missing global variable names
Use output.globals to specify browser global variable names corresponding to external modules
buffer (guessing 'require$$0')
readable-stream (guessing 'require$$0$1')
stream (guessing 'require$$0$2')
string_decoder (guessing 'require$$2')
created dist/ramp-crypto-address-validator.umd.js, dist/ramp-crypto-address-validator.es5.js in 3.1s
This is my first time with rollup (and karma as well), so I have some questions regarding the warnings:
Creating a browser bundle that depends on "buffer", "stream" and "string_decoder". You might need to include https://github.com/snowpackjs/rollup-plugin-polyfill-node
should we include this module?
(!) Plugin node-resolve: preferring built-in module (...) pass 'preferBuiltins: false' to disable this behavior or 'preferBuiltins: true' to disable this warning
Do we want to use built-ins or no? Please set the config appropriately.
(!) Missing global variable names
Is this something that we should fix?
Imho it is better to add them while using because other libs/deps can yous global polyfills also
In our case this is because we don't include polyfills inside package. So imho no. |
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.
LGTM, we'll fix potential errors if we find them when using the lib.
I recently checked if we can somehow move from karma to anything else but did not found anything. |
Changes
Docs
Lib requires fallbacks/polyfills/shimming to be used in the browser because the most popular cryptographic libs are written to be used in node (not browser)
webpack example