Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Refactory cryptography util - closes #516 #589

Merged
merged 7 commits into from
Aug 1, 2018

Conversation

yatki
Copy link
Contributor

@yatki yatki commented Jul 31, 2018

Continuation of this PR: #587

Review checklist

@yatki yatki self-assigned this Jul 31, 2018
@yatki yatki requested review from shuse2 and willclarktech July 31, 2018 13:57
package.json Outdated
@@ -76,7 +78,7 @@
"chalk": "2.3.0",
"cli-table2": "LiskHQ/cli-table2#187c6ae80b9d963c598d0cb3379153387bfb15c4",
"inquirer": "6.0.0",
"lisk-elements": "1.0.0-beta.3",
"lisk-elements": "1.0.0-rc.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think rc.1 is already out, so i think we can use the latest one

"config": { "description": "Manages Lisk Commander configuration." }
"config": {
"description": "Manages Lisk Commander configuration."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems unrelated.

Copy link
Contributor

Choose a reason for hiding this comment

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

prettify seems to have some random behavior.
anyone know why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@willclarktech @shuse2 I realized when you run npm install it formats package.json. I upgraded lisk-elements with npm command thus it introduced this change. I say we should keep this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, with npm v5 it preserves indentation as well I think. OK!

Copy link
Contributor

@willclarktech willclarktech left a comment

Choose a reason for hiding this comment

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

LGTM, but I just had a thought: can we remove the util file entirely? It doesn't seem to add much value.

@shuse2
Copy link
Contributor

shuse2 commented Aug 1, 2018

@willclarktech

LGTM, but I just had a thought: can we remove the util file entirely? It doesn't seem to add much value.

util file u mean this crypto util? I feel the same way.
Only value that i can think of is that it's still external library, so it might be good to have wrapper function in case there are breaking change. (easier to fix)
So how abt remove gradually when migrating?

@willclarktech
Copy link
Contributor

it's still external library, so it might be good to have wrapper

A good point in general, but I don't think we use many of these functions more than once.

@shuse2 shuse2 merged commit c48ebbf into 2.0.0 Aug 1, 2018
@shuse2 shuse2 deleted the 516-refactory-cryptography-util branch August 1, 2018 09:51
@yatki yatki mentioned this pull request Aug 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants