[ZNS ZChain] String Resolver #106

MichaelKorchagin · 2024-05-07T20:37:48Z

No description provided.

openzeppelin-code · 2024-05-07T20:41:15Z

[ZNS ZChain] String Resolver

Generated at commit: 898ac6067eaf1a47f790cc74fe72148170b23283

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 1 0 2 20 24
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

Whytecrowe · 2024-05-07T21:02:06Z

contracts/resolver/IZNSStringResolver.sol

+
+interface IZNSStringResolver {
+    /**
+     * @param newString The new domain owner


change the order of these.
also, newString comment is not correct. it's not the owner, it's the string that owner assigns which a domain will resolve to

Whytecrowe · 2024-05-07T21:04:14Z

contracts/resolver/ZNSStringResolver.sol

+        require(
+            registry.isOwnerOrOperator(domainHash, msg.sender) ||
+            // TODO: decide, whether the registrar can set a string
+            accessController.isRegistrar(msg.sender),


please, remove the comment and L63. Registrar will not be able to set a string. only owner or operator can.

Whytecrowe · 2024-05-07T21:06:55Z

test/ZNSStringResolver.test.ts

+
+
+describe("ZNSStringResolver", () => {
+  describe("One campaign for everybody", () => {


rename to "Single State Tests"

Whytecrowe · 2024-05-07T21:07:56Z

test/ZNSStringResolver.test.ts

+      ] = await hre.ethers.getSigners();
+
+      const campaignConfig = await getConfig({
+        deployer: deployer as unknown as SignerWithAddress,


this probably doesn't need cast to unknown. can just cast as SignerWithAddress right away, without unknown

Whytecrowe · 2024-05-07T21:08:43Z

test/ZNSStringResolver.test.ts

+      await mongoAdapter.dropDB();
+    });
+
+    it("Should not let initialize the contract", async () => {


add "twice" to the test name.
"Should not let initialize the contract twice"

Whytecrowe · 2024-05-07T21:09:36Z

test/ZNSStringResolver.test.ts

+        distrConfigEmpty,
+        paymentConfigEmpty,
+      );
+      await stringResolver.connect(user).setString(domainNameHash, "hippopotamus");


put "hippopotamus" into a var and then compare to this var in expect. don't hardcode stuff

Whytecrowe · 2024-05-07T21:09:50Z

test/ZNSStringResolver.test.ts

+      expect(
+        await stringResolver.resolveDomainString(domainNameHash)
+      ).to.eq(
+        "hippopotamus"


put the var here

Whytecrowe · 2024-05-07T21:11:03Z

test/ZNSStringResolver.test.ts

+        .to.emit(stringResolver, "RegistrySet")
+        .withArgs(admin.address);
+
+      expect(await stringResolver.registry()).to.equal(admin.address);


since you have a single state (a single deployment of ZNS), reset the Registry here to the proper address after all the checks are done, otherwise other tests would fail. registry needs to be correct in order for next tests to work properly

Whytecrowe · 2024-05-07T21:29:34Z

test/ZNSStringResolver.test.ts

+      await mongoAdapter.dropDB();
+    });
+
+    it("Should not allow non-owner address to setString (similar domain and string)", async () => {


what does "(similar domain and string)" mean? is there a reason you made the parent and child domain the same string?

Just different test params to cover more scenarios

Whytecrowe · 2024-05-07T21:30:48Z

test/ZNSStringResolver.test.ts

+      });
+
+      await expect(
+        stringResolver.connect(user).setString(hashDomainLabel(curStringDomain), curStringDomain)


I don't understand what this does and why.

Whytecrowe · 2024-05-07T21:32:35Z

test/ZNSStringResolver.test.ts

+    });
+
+    it.skip("Should allow REGISTRAR_ROLE to setString and emit event." +
+    "TODO: decide, whether the registrar can set a string  (different domain and string)", async () => {


don't add "TODOs" to the test name. always add them as comments.
and no, Registrar will not be able to set a string through registration. so you can delete this test altogether AFTER you deleted the registrar line from the Solidity code that checks registrar role in setString()

Whytecrowe

Please, address the comments and fix other failing tests in the repo.
Good start!

codecov-commenter · 2024-05-08T22:54:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.81%. Comparing base (449e121) to head (898ac60).
Report is 20 commits behind head on zns-zchain-final.

Additional details and impacted files

@@                Coverage Diff                @@
##           zns-zchain-final     #106   +/-   ##
=================================================
  Coverage             99.80%   99.81%           
=================================================
  Files                    11       12    +1     
  Lines                   525      547   +22     
  Branches                117      122    +5     
=================================================
+ Hits                    524      546   +22     
  Misses                    1        1

JamesEarle · 2024-05-20T22:10:41Z

contracts/resolver/ZNSStringResolver.sol

+    ERC165,
+    IZNSStringResolver {
+
+    mapping(bytes32 domainHash => string resolvedString) internal domainStrings;


the name domainStrings could be a bit misleading in that it implies the resolved strings are the domains themselves, not what they resolve to, maybe resolvedStrings similar to the name you already are using in the mapping key/value pair

Called them resolvedString and resolvedStrings, which I dont like, to be honest. Tell me, if I'm wrong and have to find another name)

JamesEarle · 2024-05-20T22:16:29Z

contracts/resolver/ZNSStringResolver.sol

+        string calldata newString
+    ) external override {
+        // only owner or operator of the current domain can set the string
+        require(


Using custom errors is more gas efficient. I know they aren't used in the other contracts but they have already been deployed, so maybe we can update that at some point but for now use them instead of require and do the opposite logic of what you're checking here

if (!registry.IsOwnerOrOperator(domainHash, msg.sender)) { revert CustomErrorName(); }

JamesEarle · 2024-05-20T22:21:45Z

src/deploy/constants.ts

@@ -28,4 +28,7 @@ export const EXECUTOR_ROLE = ethers.solidityPackedKeccak256(

 export const ResolverTypes = {


this would be better as an enum, then it would resolve the "TODO" comment you have below as these are just given values 0, 1, 2 etc. unless you specifically assign them something.

export enum ResolverTypes { ADDRESS, STRING }

that would require a Registry contract upgrade...

JamesEarle · 2024-05-20T22:41:41Z

test/ZNSStringResolver.test.ts

+        );
+      });
+
+      // TODO: Falls on the role. I think, cannot give a REGISTRAR_ROLE to mock "deployAdmin".


Make sure you take care of any existing TODO comments. If they are already fixed, remove them

…ngs mapping

MichaelKorchagin added 3 commits March 16, 2024 16:21

String Resolver init

3662fac

SR mission added

9f774a9

Tests for SR

30d6e95

MichaelKorchagin requested review from JamesEarle and Whytecrowe May 7, 2024 20:37

Add DB drop to test, fix naming

ef3b152

Whytecrowe reviewed May 7, 2024

View reviewed changes

Linter fixes

a237e61

Whytecrowe reviewed May 7, 2024

View reviewed changes

Whytecrowe requested changes May 7, 2024

View reviewed changes

Whytecrowe changed the title ~~Feat/zns string resolver~~ [WIP] String Resolver May 7, 2024

MichaelKorchagin added 3 commits May 8, 2024 15:35

Adjusting the deployment process. Сorrections based on review

b625249

Operator initialize

e3ee2c3

Only method deletion

09fed3b

JamesEarle reviewed May 20, 2024

View reviewed changes

Replacing require with a custom error. New names of variables in stri…

96de554

…ngs mapping

Whytecrowe changed the title ~~[WIP] String Resolver~~ String Resolver Jul 2, 2024

Whytecrowe changed the base branch from development to rc/zns-meowchain July 2, 2024 21:25

Whytecrowe added 8 commits July 2, 2024 14:48

Merge branch 'rc/zns-meowchain' into feat/zns-string-resolver

a7e70c9

change error to the existing one

76925f9

update solidity pragmas

2f48b22

update mock pragma

f9d396d

fix small errors

71cceaf

some test fixes

e785ccf

change test logic to adhere to the newest errors

953fc73

fix linting

bbb8136

Whytecrowe changed the title ~~String Resolver~~ [RC Meowchain] String Resolver Jul 17, 2024

Whytecrowe changed the title ~~[RC Meowchain] String Resolver~~ [RC ZChain] String Resolver Sep 11, 2024

Whytecrowe changed the title ~~[RC ZChain] String Resolver~~ [ZNS ZChain] String Resolver Sep 11, 2024

Whytecrowe changed the base branch from rc/zns-meowchain to zns-zchain-final September 11, 2024 22:23

Whytecrowe added 2 commits September 11, 2024 15:30

Merge branch 'zns-zchain-final' into feat/zns-string-resolver

d689dcc

fix some types

898ac60

Whytecrowe merged commit 1b7cf47 into zns-zchain-final Sep 11, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZNS ZChain] String Resolver #106

[ZNS ZChain] String Resolver #106

MichaelKorchagin commented May 7, 2024

openzeppelin-code bot commented May 7, 2024 •

edited

Loading

Whytecrowe May 7, 2024

Whytecrowe May 7, 2024

Whytecrowe May 7, 2024

Whytecrowe May 7, 2024

Whytecrowe May 7, 2024

Whytecrowe May 7, 2024

Whytecrowe May 7, 2024

Whytecrowe May 7, 2024 •

edited

Loading

Whytecrowe May 7, 2024 •

edited

Loading

MichaelKorchagin May 8, 2024

Whytecrowe May 7, 2024

Whytecrowe May 7, 2024 •

edited

Loading

Whytecrowe left a comment

codecov-commenter commented May 8, 2024 •

edited by codecov bot

Loading

JamesEarle May 20, 2024

MichaelKorchagin May 30, 2024

JamesEarle May 20, 2024

JamesEarle May 20, 2024

Whytecrowe May 29, 2024

JamesEarle May 20, 2024



		describe("ZNSStringResolver", () => {
		describe("One campaign for everybody", () => {

		@@ -28,4 +28,7 @@ export const EXECUTOR_ROLE = ethers.solidityPackedKeccak256(

		export const ResolverTypes = {

[ZNS ZChain] String Resolver #106

[ZNS ZChain] String Resolver #106

Conversation

MichaelKorchagin commented May 7, 2024

openzeppelin-code bot commented May 7, 2024 • edited Loading

[ZNS ZChain] String Resolver

🚨 Report Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Whytecrowe May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Whytecrowe May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Whytecrowe May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Whytecrowe left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 8, 2024 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openzeppelin-code bot commented May 7, 2024 •

edited

Loading

Whytecrowe May 7, 2024 •

edited

Loading

Whytecrowe May 7, 2024 •

edited

Loading

Whytecrowe May 7, 2024 •

edited

Loading

codecov-commenter commented May 8, 2024 •

edited by codecov bot

Loading