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

reduced lib size from 738,548 -> 438,991 #31

Merged
merged 6 commits into from
Mar 4, 2020

Conversation

jay-dee7
Copy link
Contributor

Signed-off-by: Jasdeep Singh jasdeepsingh.uppal@gmail.com

@jay-dee7
Copy link
Contributor Author

@Arachnid tiny improvements on #26

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>
Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>
Copy link
Member

@Arachnid Arachnid 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! Repackaging others' work is going to require a few changes, though:

  • Each contribution needs to cite the library it was taken from, and have an appropriate license included in the header of the file as a comment, or as a separate LICENSE file.
  • All of the source libraries and licenses need to be cited in the project's own README or LICENSE file.
  • You need to check that all the licenses allow reuse and modification (they likely do).
  • You need to check that the licenses are compatible (eg, they don't have any clauses that prohibit using code under difference licenses together in the same library).

Finally, since this is mostly of use to us, it'd be great if you could transfer the repo and the NPM package to @ensdomains.

package.json Outdated
@@ -10,6 +10,7 @@
"lint": "tslint -p tsconfig.json",
"prepare": "npm run build",
"prepublishOnly": "npm run test",
"sized": "browserify src/internal/index.js -p [tsify --noImplicitAny] | wc -c",
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this?

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'll remove it. initially i was planning to use an internal directory instead of an npm package

package.json Outdated
"rskjs-util": "^1.0.3",
"stellar-base": "^2.1.2",
"crc": "^3.8.0",
"crypto-addr-codec": "^0.1.1",
Copy link
Member

Choose a reason for hiding this comment

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

Can you please pin this to a specific version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, can i include a yarn.lock file? or just by using the version would be okay?

Copy link
Member

Choose a reason for hiding this comment

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

Just using the version is fine.

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 1, 2020

@Arachnid i'll check with all the packages and see if we can include their license without any issues and i'm more than happy to transfer the ownership of the repo :)

@inspireme6
Copy link
Contributor

Since @Arachnid is talking about transfer your repo, can you @jay-dee7 include the ss58, bs58, cashaddr and shajs files into the repo to keep it all in one place? Thanks

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 2, 2020

@inspireme6 sure. can you make a PR to the crypto-addr-serialize repo. I see you've made some changes?

@inspireme6
Copy link
Contributor

@inspireme6 sure. can you make a PR to the crypto-addr-serialize repo. I see you've made some changes?

The only significant change was referencing the shajs modified in the package.json.. I had to add the latest files you uploaded (the stellar files) because they disappeared for some reason (?). The modified shajs package is here https://github.com/inspireme6/sha256js

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 2, 2020

@inspireme6 sure. can you make a PR to the crypto-addr-serialize repo. I see you've made some changes?

The only significant change was referencing the shajs modified in the package.json.. I had to add the latest files you uploaded (the stellar files) because they disappeared for some reason (?). The modified shajs package is here https://github.com/inspireme6/sha256js

@inspireme6 thats my bad, i published it directly from my local work, i didn't push it to the repo. sorry. can you please rebase it from master and update the PR?

@inspireme6
Copy link
Contributor

inspireme6 commented Mar 2, 2020

@inspireme6 sure. can you make a PR to the crypto-addr-serialize repo. I see you've made some changes?

The only significant change was referencing the shajs modified in the package.json.. I had to add the latest files you uploaded (the stellar files) because they disappeared for some reason (?). The modified shajs package is here https://github.com/inspireme6/sha256js

@inspireme6 thats my bad, i published it directly from my local work, i didn't push it to the repo. sorry. can you please rebase it from master and update the PR?

@jay-dee7 I see you have pushed the changes already, it's necessary to rebase it anyways? I'm still new to git, so not sure.

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 2, 2020

No its not. I'll do it dont worry 😁

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 3, 2020

@Arachnid i've sent you an invitation for the github repo, please accept it and migrate the repository it to @ensdomains. Since i dont have permissions at ensdomains, i cant transfer.

Also, once the repository is migrated, i can un-publish the npm package from my account and you should be able to publish it with the same name or a different one, however you see fit.

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>
@jay-dee7 jay-dee7 changed the title reduced lib size from 738,548 -> 481,306 reduced lib size from 738,548 -> 438,991 Mar 3, 2020
@Arachnid
Copy link
Member

Arachnid commented Mar 3, 2020

@jay-dee7 I now have push access, but you need to add me as an owner before I can transfer it.

@@ -226,11 +230,8 @@ function b32encodeXemAddr(data: Buffer): string {
}

function b32decodeXemAddr(data: string): Buffer {
if(!isValid(data)) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to remain in, unless you have something else to check validity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding it right back

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 3, 2020

@jay-dee7 I now have push access, but you need to add me as an owner before I can transfer it.

i couldn't make you an owner, i think that option is only for organizations, i've sent an invitation to your user account @Arachnid

Signed-off-by: Jasdeep Singh <jasdeepsingh.uppal@gmail.com>
@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 3, 2020

this validation check put me back in time :(
latest size as it shows in travis logs is 440,007

@Arachnid
Copy link
Member

Arachnid commented Mar 3, 2020

@jay-dee7 Thanks, I've accepted the transfer. Are you able to transfer the NPM package to me or to the ensdomains org?

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 3, 2020

@Arachnid I'll let you know in next 20 minutes

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 4, 2020

@jay-dee7 Thanks, I've accepted the transfer. Are you able to transfer the NPM package to me or to the ensdomains org?

@Arachnid just added you as a maintainer for the npm package, please confirm it and i'll remove myself from the list

@jay-dee7
Copy link
Contributor Author

jay-dee7 commented Mar 4, 2020

@Arachnid i see that both, the repository and the npm package is under @ensdomains organization now. Please let me know if there's something, blocking this PR.

@inspireme6
Copy link
Contributor

inspireme6 commented Mar 4, 2020

@Arachnid I just made a PR to the crypto-addr-serialize package that includes the rest of the encoders (bs58 and cashaddr) and includes the sha256 script which was part of the latest changes I made. Also includes the license info for the ss58, cashaddr and shajs packages.

After @jay-dee7 PR gets merged, I'll make a PR to this repo that will update the files to include the changes from the updated crypto-addr-serialize package if it goes through.

@Arachnid Arachnid merged commit e9d627b into ensdomains:master Mar 4, 2020
momodaka pushed a commit to momodaka/address-encoder that referenced this pull request Mar 7, 2020
reduced lib size from 738,548 -> 438,991
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.

None yet

3 participants