Skip to content

feat: L2Resolver state variables and constructor w/unit tests - #2

Merged
0xItadaki merged 13 commits into
devfrom
feat/L2Resolver-contract-constructor
Aug 19, 2025
Merged

feat: L2Resolver state variables and constructor w/unit tests#2
0xItadaki merged 13 commits into
devfrom
feat/L2Resolver-contract-constructor

Conversation

@0xItadaki

Copy link
Copy Markdown
Collaborator

🤖 Linear

Closes EFI-425
Closes EFI-434

@linear

linear Bot commented Aug 19, 2025

Copy link
Copy Markdown
EFI-425 constructor and variables

State Variables:

  • mapping(bytes32 _node => mapping(string _key => bytes _data)) public records;
    • Stores arbitrary data for each ENS node.
    • For domain-to-chain-identifier resolution, the key will be a pre-defined string (e.g., CHAIN_IDENTIFIER_EIP7930_KEY) and data will be the raw EIP-7930 chain identifier bytes (formatted with AddressLength = 0).
    • For EIP-7930-chain-identifier-to-domain resolution (reverse lookup), the REVERSE_LOOKUP_NODE is used. The key is constructed by concatenating a prefix (e.g., CHAIN_IDENTIFIER_EIP7930_KEY) with the hexadecimal string representation of the keccak256 hash of the EIP-7930 chain identifier bytes. The data stored is the ABI-encoded human-readable ENS name string (e.g., abi.encode("optimism.l2.eth")).

Constants:

  • bytes32 constant REVERSE_LOOKUP_NODE = bytes32(keccak256("reverse.chain.id.eip7930"));
    • A dedicated node hash for storing reverse lookup entries within this contract's records mapping.
  • string constant CHAIN_IDENTIFIER_EIP7930_KEY = "chain.id.eip7930";
    • The key used in records to store the EIP-7930 chain identifier bytes for a forward resolution.

EFI-434 constructor

string public constant CHAIN_IDENTIFIER_EIP7930_KEY = 'chain.id.eip7930';

/// @inheritdoc IL2Resolver
IENS public immutable ENS_REGISTRY;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xAustrian what do you think about the visibility of these variables? I went with public so I could unit test them. I kept _REVERSE_LOOKUP_NODE since it will be used in internal logic

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would set everything on public tbh, even REVERSE_LOOKUP_NODE


/**
* @title L2Resolver
* @author @defi-wonderland

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we usually set the author?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sir

Base automatically changed from chore/clean-boilerplate-files to dev August 19, 2025 12:43
@0xItadaki
0xItadaki requested a review from 0xAustrian August 19, 2025 12:44
Comment thread src/contracts/L2Resolver.sol Outdated
@@ -0,0 +1,51 @@
// SPDX-License-Identifier: MIT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty space here

Comment thread src/contracts/L2Resolver.sol Outdated
@@ -0,0 +1,51 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.30;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a fixed .sol version.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it's no big deal but a good practice to use the exact same version everywhere.

Comment thread test/integration/L2Resolver.sol Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so the CI does not fail, right? In that case I think we can temporarily comment the integration tests section in the workflow (sorry, forgot to mention that. It would have been a good addition to the PR#1).

@0xItadaki
0xItadaki requested a review from 0xAustrian August 19, 2025 13:09

@0xAustrian 0xAustrian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

@0xItadaki
0xItadaki merged commit e1d50be into dev Aug 19, 2025
4 checks passed
@0xItadaki
0xItadaki deleted the feat/L2Resolver-contract-constructor branch August 19, 2025 13:13
@0xAustrian
0xAustrian restored the feat/L2Resolver-contract-constructor branch August 19, 2025 14:16
@0xItadaki 0xItadaki self-assigned this Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants