-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REG-1154] - Add ENS DNSSEC smart contracts to configs, artifacts and sandbox #310
Conversation
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.
Slither found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
Contracts size report
|
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #310 +/- ##
===========================================
+ Coverage 86.92% 98.70% +11.77%
===========================================
Files 34 20 -14
Lines 1239 1000 -239
Branches 181 217 +36
===========================================
- Hits 1077 987 -90
+ Misses 162 13 -149 see 14 files with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
@@ -0,0 +1,166 @@ | |||
pragma solidity ^0.8.4; |
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.
Shall we include links to initial sources for all of this contracts?
* proof must be the TXT record required by the registrar. | ||
* @param proof The proof record for the first element in input. | ||
*/ | ||
function proveAndClaim( |
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 it makes sense to add a test minting a DNS-proved ENS domain to ensure valid configuration. Probably in sandbox/index.test.ts
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.
LGTM apart from some minor stuff
PR Checklist
1. Contracts versioning
patch
version of the contracts is increased if changes have been made to theUNSRegistry
,MintingManager
,ProxyReader
,ENSCustody
contracts.minor
version of the contracts is increased if breaking changes have been made to theUNSRegistry
,MintingManager
,ProxyReader
,ENSCustody
contracts. It includes changes of interfaces.2. Contracts licensing
3. Coverage
4. Configs versioning
uns-config.json
is increased if changes have been made to the config.ens-config.json
is increased if changes have been made to the config.resolver-keys.json
is increased if changes have been made to the config.ens-resolver-keys.json
is increased if changes have been made to the config.5. Package versioning
patch
version of package is increased if valuable changes have been made to the package. It includes contracts update, configs update, etc.major.minor
version of package is synced with version ofUNSRegistry
contract.CHANGELOG
is updated with short description for the new version.6. Code review
resolver-keys.json
code review is required from DevTools teamens-resolver-keys.json
code review is required from DevTools team