Skip to content
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

ENS Input Portal #260

Closed
guidanoli opened this issue Apr 4, 2024 · 15 comments · Fixed by #275
Closed

ENS Input Portal #260

guidanoli opened this issue Apr 4, 2024 · 15 comments · Fixed by #275
Assignees
Labels
A-contracts Area: contracts T-feature Type: feature
Milestone

Comments

@guidanoli
Copy link
Collaborator

📚 Context

In the Ethereum ecosystem, ENS names are often used as aliases for addresses, as they can be shorter and more readable than the standard 20-byte hexes. If applications allowed users to authenticate through their ENS names, they would be able to sell their accounts via their ENS names. This is much safer and seamless than exposing their private key.

✔️ Solution

Create a new contract called EnsInputPortal, which takes an application contract address, an ENS name and a payload, verifies that the ENS name resolves to the message sender's address, and adds an input to the application's input box with the ENS name and payload.

The input payload should be encoded using abi.encode since both fields have variable length. In the back-end, the application will be able to decode the input payload using any Ethereum library that has a similar function to Solidity's abi.decode.

@guidanoli guidanoli added T-feature Type: feature A-contracts Area: contracts labels Apr 4, 2024
@guidanoli guidanoli mentioned this issue Apr 4, 2024
@guidanoli
Copy link
Collaborator Author

Observation: We don't have to create portals for asset deposits using ENS names. Developers can use the execLayerData field from inputs added by regular portals to encode an optional destination ENS name.

The downside is that there won't be a standard API for depositing assets to ENS names. If this feature gets used a lot in the future, we might consider creating portals for ENS names. It will be then a minor (non-breaking) change, as only new contracts will be added.

@guidanoli guidanoli added this to the 2.0.0 milestone Apr 18, 2024
@ZzzzHui
Copy link
Contributor

ZzzzHui commented Apr 24, 2024

Should this issue be closed? Or is it still useful

@guidanoli
Copy link
Collaborator Author

It is still useful!
Otherwise, how would users identified by their ENS names perform authorized actions?

@ZzzzHui
Copy link
Contributor

ZzzzHui commented Apr 26, 2024

The input payload should be encoded using abi.encode since both fields have variable length. In the back-end, the application will be able to decode the input payload using any Ethereum library that has a similar function to Solidity's abi.decode.

abi.encodePacked is fine because onchain codes usually only deal with bytes32 node rather than ENS name.

@ZzzzHui ZzzzHui linked a pull request Apr 26, 2024 that will close this issue
@guidanoli
Copy link
Collaborator Author

But then how do you go from bytes32 namehash to the actual string name?

@ZzzzHui
Copy link
Contributor

ZzzzHui commented Apr 29, 2024

But then how do you go from bytes32 namehash to the actual string name?

We don't. When authenticating, we could check if bytes32 matches right?

@guidanoli
Copy link
Collaborator Author

I'm thinking about the front-end. How would it display the ENS-identified user? If we displayed just the ENS node hash, we would have no readability benefit. In fact, it would be worse than using Ethereum addresses, because we would go from 20 to 32 bytes.

I'm thinking the ENS Input Portal should also include the ENS name in the input. In this way, the application could externalize both the ENS node hash and name through GraphQL (through reports or notices). Vouchers would only work with ENS node hashes, of course.

@ZzzzHui ZzzzHui self-assigned this Apr 29, 2024
@ZzzzHui
Copy link
Contributor

ZzzzHui commented Apr 29, 2024

The key is, before calculating the namehash, a name has to be normalized. I don't think there's an efficient way for name normalization onchain at the moment. Especially if we are thinking about doing this name normalization repeatedly and regularly. There must be a reason why ENS contracts are working with bytes32 node rather than the name directly, right?

I think the solution should be offchain, i.e., name processing should be handled offchain. For example, at frontend, we could consider passing the normalized name as part of the payload execLayerData.

So, ENS input portal shouldn't be used on a regular basis. Instead, users should call the regular addInput function as much as possible, and leave logics to offchain as much as possible.

@guidanoli
Copy link
Collaborator Author

I don't think there's an efficient way for name normalization onchain at the moment.
I think the solution should be offchain

I agree, the ENS input portal should accept not only the ENS node but also the name, and include it in the input. Then, the application back-end should check whether the ENS name and node match, and revert otherwise.

So, ENS input portal shouldn't be used on a regular basis. Instead, users should call the regular addInput function as much as possible, and leave logics to offchain as much as possible.

I partially agree. In some cases, there is no need to add inputs through the ENS portal. For example, deposits can easily go through standard portals and the recipient's ENS name can be encoded in the execLayerData field. You don't need authorization to receive deposits, right?

Meanwhile, some actions require explicit authorization, and, in the case of ENS-identified users, must go through the ENS input portal. For example, withdrawal requests, moving a piece in the chess board, etc.

@ZzzzHui
Copy link
Contributor

ZzzzHui commented Apr 29, 2024

Meanwhile, some actions require explicit authorization, and, in the case of ENS-identified users, must go through the ENS input portal. For example, withdrawal requests, moving a piece in the chess board, etc.

What I have in mind is maybe call the ENS portal once for the backend to establish a mapping between Ethereum address and ENS name/node. From then on, simply call the regular addInput function. Until there's a change to the ENS resolution (e.g. ENS is expired or sold). Then the new ENS owner would call this ENS portal again to update the mapping to a new address. Then again, regular addInput function would suffice.

I need to think more about this to see if it's safe. I'm sure what you have in mind is much safer but also a bit more costly.

@guidanoli
Copy link
Collaborator Author

guidanoli commented Apr 30, 2024

What I have in mind is maybe call the ENS portal once for the backend to establish a mapping between Ethereum address and ENS name/node.

We have already explored this solution in #58, and I highly encourage you to revisit the discussion in the issue. To summarize, we've agreed to only resolve ENS nodes in the base layer, because the execution layer is always prone to having an outdated view of the ENS registry, which can lead to undesired behavior both in the back-end and front-end.

@guidanoli
Copy link
Collaborator Author

I think the solution should be offchain, i.e., name processing should be handled offchain. For example, at frontend, we could consider passing the normalized name as part of the payload execLayerData.

I 100% agree that validation should be delegated to the execution layer, to reduce the gas, and therefore fees, spent on the base layer as much as possible.

Now, regarding how the ENS name is encoded in the payload is a topic I'd like to raise. We could let applications and HLFs come up with ways of encoding ENS names into execLayerData, but I believe it would be beneficial for it to be a separate field. I'm assuming that applications that support ENS as an identification method would very much like to display ENS names rather than node hashes. So, having the ENS name as a separate field would help not only the front-end, which wouldn't need to encode the ENS name in the execLayerData blob, and have a proper, typed, parameter for it; but, more expressively, the back-end, which wouldn't need to know the encoding of the execLayerData field in order to validate the ENS name and node fields.

@ZzzzHui
Copy link
Contributor

ZzzzHui commented May 3, 2024

Yes we could have ENS names as a separate field. Or to be more consistent with other encoding methods in the InputEncoding library, we can have the following:

    function encodeENSInput(
        bytes32 node,
        bytes calldata ensName,
        bytes calldata execLayerData
    ) internal pure returns (bytes memory) {
        bytes memory data = abi.encode(ensName, execLayerData);
        return
            abi.encodePacked(
                node, 
                data 
            );
    }

So the 2 dynamic variables would be abi.encoded as data. And the return statement still returns abi.encodePacked. This return structure is basically the same as input encodings for ERC721 and ERC1155. What do you think?

@guidanoli
Copy link
Collaborator Author

guidanoli commented May 3, 2024

Exactly, though I would rewrite it as a single abi.encode.

    function encodeENSInput(
        bytes32 node,
        bytes calldata name,
        bytes calldata execLayerData
    ) internal pure returns (bytes memory) {
        return
            abi.encode(
                node,
                name,
                execLayerData
            );
    }

You'll notice that it will output the same encoding as the one you suggested, but it's easier to read.
I've renamed ensName to just name, because we're not calling the ENS node ensNode, but just node.

@ZzzzHui
Copy link
Contributor

ZzzzHui commented May 6, 2024

Yes I agree with you. I will modify as you suggested. Thanks!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contracts Area: contracts T-feature Type: feature
Projects
Status: 🚀 Done
Development

Successfully merging a pull request may close this issue.

2 participants