Conversation
|
please don't include other PR under review. please rebase it to the other branch and set this PR's base to that branch until it gets merged. |
molekilla
force-pushed
the
dapp-tcr
branch
2 times, most recently
from
c07a3d4 to
2cc22af
Compare
feat(contracts): rename sha3 method, apply recommend suggestions from code review feat(contracts): FDSRegistry is ENSRegistry, supports @ensdomains interface. fix prettier issues. removed subdomain feat(contracts): FDSRegistry is ENSRegistry, supports @ensdomains interface. fix prettier issues. removed subdomain feat(contracts): removing fdsregistry, not needed (typechain builds artifacts from npm packages ie ENSRegistry) feat(contracts): fixes
|
@nugaon ready for review |
nugaon
left a comment
nugaon
left a comment
There was a problem hiding this comment.
I have many concerns about the current implementation, please read my comments below.
Also, remove all ERC20 token related code, I don't know why did you commit those.
There are no methods in DappRegistry to change the Record value of a dApp by the owner.
contracts/TestToken.sol
Outdated
|
|
||
| import "./ERC20/ERC20.sol"; | ||
|
|
||
| contract TestToken is ERC20 { |
There was a problem hiding this comment.
This was the previous code based on token registry old design pattern that used ERC20 (I change to use ENS base registry in last commit). Will remove.
| // indexation type | ||
| uint8 indexType; | ||
| // Data structure format | ||
| bytes32 dataFormat; |
There was a problem hiding this comment.
can you elaborate this one please? it is related to BeeSon or any other datastructure signiture?
There was a problem hiding this comment.
I think we should leave this agnostic, beeson by default, but they could use other dataformats. If is always beeson, then I wil remove it
There was a problem hiding this comment.
eg based on the multibase tables.csv values
There was a problem hiding this comment.
no it is fine as it is for now
contracts/DappRegistry.sol
Outdated
| bytes32 indexed node, | ||
| bytes32 label, | ||
| uint256 duration, | ||
| address indexed owner |
There was a problem hiding this comment.
why should we index owner?
| bytes32 indexed node, | |
| bytes32 label, | |
| uint256 duration, | |
| address indexed owner | |
| bytes32 indexed node, | |
| bytes32 label, | |
| uint256 duration, | |
| address owner |
There was a problem hiding this comment.
This is probably from the old design pattern, with ENS not required
contracts/DappRegistry.sol
Outdated
| event TransferRecord( | ||
| address from, | ||
| address to, | ||
| bytes32 node |
There was a problem hiding this comment.
maybe index node?
| bytes32 node | |
| bytes32 indexed node |
|
|
||
|
|
||
| // Global Variables | ||
| ENS public ensInstance; |
yarn.lock
Outdated
| @@ -0,0 +1,11492 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
test/DappRegistry.ts
Outdated
| dappRegistry = await DappRegistry.deploy(ens.address, registrar.address, keccak256FromUtf8Bytes('dappregistry')) | ||
| await dappRegistry.deployed() | ||
| // set subnode owner and add to controller | ||
| await registrar.setSubnodeOwner(keccak256FromUtf8Bytes('dappregistry'), dappRegistry.address) |
There was a problem hiding this comment.
I got error:
Property 'setSubnodeOwner' does not exist on type 'FDSRegistrar'.ts(2339)
contracts/DappRegistry.sol
Outdated
| bytes32 _node | ||
| ) { | ||
| ensInstance = _ens; | ||
| baseNode = _node; |
There was a problem hiding this comment.
why do you need this baseNode? you don't use it for anything.
test/DappRegistry.ts
Outdated
|
|
||
| const DappRegistry = await ethers.getContractFactory('DappRegistry') | ||
| // set ENS and FDS addresses, and configure subdomain (eg dappregistry.fds) | ||
| dappRegistry = await DappRegistry.deploy(ens.address, registrar.address, keccak256FromUtf8Bytes('dappregistry')) |
There was a problem hiding this comment.
I imagined like the names of dapps are on the same level as normal user names. e.g. nugaon.fds a username, then anythread.fds can be a dapp name
There was a problem hiding this comment.
let me make that change then
contracts/DappRegistry.sol
Outdated
| // ENS app name | ||
| bytes32 node; | ||
| // owner | ||
| address owner; |
There was a problem hiding this comment.
this should be tracked by the ENS contract.
…pp update, cleaning package.json/lock and fixes to unit tests
|
@nugaon will update this PR later this week |
3 participants
Refactors to use ENS ERC721 token curated registry on top of FDS registrar.
Not implemented in this release/issue: deployment or migration scripts