Add ENSEncoder #8
Conversation
hewigovens
commented
Apr 25, 2018
hewigovens
commented
- Add ENSEncoder and tests
- Add Namehash and tests
- Mark TrustCoreTests scheme as shared
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
==========================================
+ Coverage 82.51% 84.03% +1.52%
==========================================
Files 20 23 +3
Lines 858 946 +88
==========================================
+ Hits 708 795 +87
- Misses 150 151 +1
Continue to review full report at Codecov.
|
| @@ -1,3 +1,6 @@ | |||
| .DS_Store | |||
There was a problem hiding this comment.
You should add this to your global .gitignore see https://help.github.com/articles/ignoring-files/
There was a problem hiding this comment.
I can revert this change, but just like we use swiftlint pod version (not global), what if other contributors not set global .gitignore?
There was a problem hiding this comment.
It's fine to leave it in.
Sources/Solidity/Namehash.swift
Outdated
|
|
||
| extension Data { | ||
| /// sha3 keccak 256 | ||
| public func sha3() -> Data { |
There was a problem hiding this comment.
Let's just use EthereumCrypto.hash(data) I think it is more explicit than data.sha3(). Also we want to have only one way of doing things.
Sources/Solidity/Namehash.swift
Outdated
| return data.sha3() | ||
| } | ||
|
|
||
| public var labelhash: Data { |
There was a problem hiding this comment.
how is this different from sha3? Why do we need a different name?
There was a problem hiding this comment.
just name convention: https://docs.ens.domains/en/latest/introduction.html#terminology
There was a problem hiding this comment.
But then it should be labelHash (capital H) and it should be a static method in the ENSEncoder class.
There was a problem hiding this comment.
how about a plain public func labelhash / namehash? it may not be used only in ENS contract (other: PublicResolver / ReverseRegistrar)
There was a problem hiding this comment.
When it's needed somewhere else we can refactor.
Sources/Solidity/Namehash.swift
Outdated
| node = self.split(separator: ".") | ||
| .map { Array($0.utf8).sha3() } | ||
| .reversed() | ||
| .reduce(node) { return ($0 + $1).sha3() } |
There was a problem hiding this comment.
This seems to be very specific, it should not be an extension to String as it contaminates the String namespace.
| @@ -0,0 +1,56 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
We don't need this scheme
There was a problem hiding this comment.
I can revert this but It's convenient for people first time opens the project, otherwise you have to click "Manage Schemes..."
There was a problem hiding this comment.
Pressing Command-U on the framework target (or any target) should run the tests. No need for a separate scheme.
There was a problem hiding this comment.
Yeah, I know that keyboard shortcuts, I just want to only write and build the tests, then do a full test when everything is done
There was a problem hiding this comment.
You can also just run some tests. Still not sure why you need a separate scheme.
There was a problem hiding this comment.
fine, I will revert this change. (it's not a separate scheme, All the tests are in the same target, I just mark it as shared. We can only run some tests but we still need to build the whole target)
There was a problem hiding this comment.
Maybe you can share your screen and show me what you mean :)
