-
Notifications
You must be signed in to change notification settings - Fork 110
contracts, swarm: implement EIP-1577 #1232
Conversation
contracts/ens/cid.go
Outdated
// Note: only CIDv1 is supported | ||
func decodeEIP1577ContentHash(buf []byte) (storageNs, contentType, hashType, hashLength uint64, hash []byte, err error) { | ||
if len(buf) < 10 { | ||
return 0, 0, 0, 0, nil, fmt.Errorf("buffer too short") |
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.
there is no format param, I guess you just need errors.New
.
contracts/ens/cid.go
Outdated
ns_ipfs = 0xe3 | ||
ns_swarm = 0xe4 | ||
|
||
swarm_typecode = 0xfa //swarm manifest |
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.
Maybe just add a comment linking to the https://github.com/multiformats/multicodec ?
) | ||
|
||
// Tests for the decoding of the example ENS | ||
func TestEIPSpecCidDecode(t *testing.T) { |
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.
Nitpick but tests like this are much more readable when they are in table-driven format, and you have to input
and wantXXX
(expectations) next to each other. Currently you have to look for const
s outside the test and you have to read carefully the test to see what's compared where - otherwise you know that all wantXXX
expectations are checked.
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.
cool. i copied the consts into the individual tests for readability sake. this specific test cannot go into the table format of the test below since it unit tests a different function. is this what you've meant?
contracts/ens/cid_test.go
Outdated
for _, v := range []struct { | ||
name string | ||
headerBytes []byte | ||
fails bool |
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 think wantErr
is a better name here, but again up to you to decide.
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.
added a few minor comments, but overall 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.
Just a minor comment about usage of snake_case variable names, maybe they should be converted to camelCase.
as featured on ethereum/go-ethereum#19285 |
from the issue below:
EIP-1062 has evolved into EIP-1577, which incorporates the use of
multiaddr
schemes into the resolver contract on ENS.This allows clients to know from which Dstorage provider to request the content addressed hash directly from the resolver.
See:
https://ethereum-magicians.org/t/eip1577-multiaddr-support-for-ens/1969
status-im/status-mobile#6688
https://eips.ethereum.org/EIPS/eip-1577
ethereum/EIPs#1577
New contract: https://github.com/ensdomains/resolvers/blob/master/contracts/PublicResolver.sol
===============
fixes #940