Structure related functions in separate source files #182

s1na · 2019-02-21T11:14:12Z

This PR follows up on the discussion in #170 (and supersedes #38) and puts conceptually related functions in separate files. I totally made up the structure. Feel free to propose different files, or suggest moving specific functions from one file to another. Here's a gist:

index: Only re-exports functions defined in other files + re-exports external modules (BN, rlp and secp256k1).
common: Utility functions such as Buffer, byte array or string manipulation
pubkey: Public-key cryptography, including managing private and public keys, and signatures
hash: Hash functions
address: Creating, validating and converting addresses. There is more overlap between this and pubkey

I haven't modified the functions themselves, except for explicitly using ethjsUtil in common instead of exports.

Update: I also removed the safe-buffer dependency.

coveralls · 2019-02-21T11:22:39Z

Coverage increased (+0.06%) to 99.327% when pulling e74c305 on refactor/sig-file into ca4eef1 on master.

holgerd77

Ok, some first review, I think this is already a very good start. Since e.g. #38 is open for more than 2 (! sigh) years I also think there is not THE absolute best structure and once we come up with something quite consistent this would already be some huge improvement.

Maybe already do some update on this PR with the change suggestions you agree with, then it becomes easier to evolve.

holgerd77 · 2019-02-21T14:07:31Z

src/address.ts

+export const isPrecompiled = function(address: Buffer | string): boolean {
+  const a = unpad(address)
+  return a.length === 1 && a[0] >= 1 && a[0] <= 8
+}


I think all this fits really well.

src/common.ts

src/hash.ts

holgerd77 · 2019-02-21T14:14:41Z

src/pubkey.ts

+
+function isValidSigRecovery(recovery: number): boolean {
+  return recovery === 0 || recovery === 1
+}


I would have a tendency to move to top-most functions to address.js and make for the others some dedicated signature.js file.

holgerd77 · 2019-02-21T14:17:31Z

src/pubkey.ts

+ */
+export const isValidPrivate = function(privateKey: Buffer): boolean {
+  return secp256k1.privateKeyVerify(privateKey)
+}


One side note: on refactoring we should go through all the file names, I think there are better versions for many of them (like this one, but also fromSigned, baToJSON, zeros) being more descriptive and generally better grasp what the function is doing. Think this would help a lot on code readability on the usage side where people (we 😄) are integrating this within their code.

s1na · 2019-02-21T14:57:02Z

Applied your comments, the main differences (to my original post) are:

Most of common renamed to buffer
defineProperties moved to own file object (and marked as deprecated)
Signature-related functions moved to signature
Public key private key functions moved together with address related functions to a new file account (instead of address, since I think account is more general)

holgerd77 · 2019-02-21T16:25:33Z

Ok, will leave this open until tomorrow so that others have a chance to comment, but I think we've already got some good ground here.

holgerd77

One file naming thing to settle out.

holgerd77 · 2019-02-22T09:49:12Z

src/account.ts

+  }
+
+  return secp256k1.publicKeyVerify(publicKey)
+}


With these two I am unsure, if they rather belong in the signature area or here. Just did some context research, both are used in wallet, I also had some short look at this HackerNoon article on Ethereum signature.

Think we can leave here for now, since we are not doing a release yet we can still move if found to be more appropriate in signature.

holgerd77 · 2019-02-22T09:53:25Z

src/buffer.ts

+  }
+
+  return ethjsUtil.isHexPrefixed(str) ? str : '0x' + str
+}


Ok, this is not fitting at all. Should we call the whole file byte.js? Then this is generally more generic and would also fit other functionality in the future eventually.

s1na · 2019-02-22T11:21:15Z

Renamed buffer to bytes. I was thinking for future maybe it'd make sense to define custom types, and organize part of the code around those types. E.g. a type Address with various constructors (static methods) such as zero(), create(), create2(), fromPublicKey(), and instance methods like toString(), toBuffer(), isValid().

holgerd77 · 2019-02-22T12:51:35Z

Hi Sina, I was out for a longer (baby-related 🍼 😄 ) walk and actually also thinking about how to better and more coherently impose hex value validation, your suggestion with custom types sounds great! I think I'll also leave function naming "as is" for this round if we might restructure in a bigger way mid-term, we should nevertheless do the API changes on this round.

Not sure how to organize some transition process, that's probably something for a second round and mid-term? Or would it already make sense to introduce parts of the functionality already here in parallel or something?

Anyhow. Will merge this PR for now. Can further brainstorm. 😄

holgerd77

Ah, small thing: can we do "byte.ts" instead of "bytes.ts"? In my ears bytes sound a bit strange and it fits more to the other namings (like account-related, hash-related, byte-related... functionality).

holgerd77 · 2019-02-22T12:56:43Z

(or if you've got a strong opinion on this please let me know)

s1na · 2019-02-22T13:09:42Z

Or would it already make sense to introduce parts of the functionality already here in parallel or something?

Maybe we can introduce the types in addition to the existing API, test it in smaller libraries and only if it worked well do a full transition?

can we do "byte.ts" instead of "bytes.ts"?

I don't have a strong opinion, and would happily change, but it does seem that in this case since we're always working with a collection of bytes and almost never with a single byte, bytes might convey the meaning better (similar packages I've seen have also been called bytes, e.g. go's bytes).

holgerd77 · 2019-02-22T13:20:50Z

Ok, let's keep, have merged.

holgerd77 · 2019-02-22T13:26:59Z

Makes sense (transition idea/suggestion).

holgerd77 reviewed Feb 21, 2019

View reviewed changes

s1na added 2 commits February 21, 2019 15:53

Mv fns to account, buffer, hash, signature and object files

339c287

Rm safe-buffer dep

c1b9281

s1na force-pushed the refactor/sig-file branch from 9fe2d76 to c1b9281 Compare February 21, 2019 14:54

holgerd77 suggested changes Feb 22, 2019

View reviewed changes

Rename buffer file to bytes

e74c305

holgerd77 suggested changes Feb 22, 2019

View reviewed changes

holgerd77 approved these changes Feb 22, 2019

View reviewed changes

holgerd77 merged commit 845a571 into master Feb 22, 2019

holgerd77 deleted the refactor/sig-file branch February 22, 2019 13:20

This was referenced Mar 12, 2019

Split off secp256k1 into its own file [WIP] #38

Closed

Introduce Address type #186

Merged

holgerd77 mentioned this pull request Apr 8, 2019

Split off the crypto functions #35

Closed

snyk-bot mentioned this pull request Nov 25, 2019

[Snyk] Upgrade ethereumjs-util from 5.2.0 to 6.2.0 ajesse11x/Civil#12

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structure related functions in separate source files #182

Structure related functions in separate source files #182

s1na commented Feb 21, 2019 •

edited

Loading

coveralls commented Feb 21, 2019 •

edited

Loading

holgerd77 left a comment

holgerd77 Feb 21, 2019

holgerd77 Feb 21, 2019

holgerd77 Feb 21, 2019

s1na commented Feb 21, 2019 •

edited

Loading

holgerd77 commented Feb 21, 2019

holgerd77 left a comment

holgerd77 Feb 22, 2019

holgerd77 Feb 22, 2019

s1na commented Feb 22, 2019

holgerd77 commented Feb 22, 2019

holgerd77 left a comment

holgerd77 commented Feb 22, 2019

s1na commented Feb 22, 2019

holgerd77 commented Feb 22, 2019

holgerd77 commented Feb 22, 2019

Structure related functions in separate source files #182

Structure related functions in separate source files #182

Conversation

s1na commented Feb 21, 2019 • edited Loading

coveralls commented Feb 21, 2019 • edited Loading

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 Feb 21, 2019

Choose a reason for hiding this comment

holgerd77 Feb 21, 2019

Choose a reason for hiding this comment

holgerd77 Feb 21, 2019

Choose a reason for hiding this comment

s1na commented Feb 21, 2019 • edited Loading

holgerd77 commented Feb 21, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 Feb 22, 2019

Choose a reason for hiding this comment

holgerd77 Feb 22, 2019

Choose a reason for hiding this comment

s1na commented Feb 22, 2019

holgerd77 commented Feb 22, 2019

holgerd77 left a comment

Choose a reason for hiding this comment

holgerd77 commented Feb 22, 2019

s1na commented Feb 22, 2019

holgerd77 commented Feb 22, 2019

holgerd77 commented Feb 22, 2019

s1na commented Feb 21, 2019 •

edited

Loading

coveralls commented Feb 21, 2019 •

edited

Loading

s1na commented Feb 21, 2019 •

edited

Loading