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

ETHRegisterControllerV2 #128

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

alextnetto
Copy link

New Features:

  • Referral for Reveal and Renew domains.
  • Batch Commit, Reveal, and Renew.
  • Payment in the commit stage with extra tips to attract the registrar Dapp to reveal the name(s), covering gas costs.

You can read more about all we did in this medium article

Since we changed a lot from the previous version, we created a V2 for the new files

  • ETHRegistrarControllerV2.sol
  • IETHRegistrarControllerV2.sol
  • TestEthRegistrarControllerV2.sol

These features will reward front-ends (registrar Dapps), increase domain registration, and enlarge the community, creating third-party services for the ENS ecosystem.

By implementing the referrer as an option in the register and renew functions, the protocol will provide commission when executing the service and share a fee previously settled by the DAO, with the referrer.

Front-end Dapps will be built to profit on the refer share. And furthermore can be done with the payment with commitment, which can incentivize a relation of trust between the final user and front-end Dapp, enabling names to be purchased with a better UX, by a single transaction.

To save even more in gas costs, the batch feature will vastly increase the intention of acquiring new names, since the gas cost will rapidly decay when purchasing more than 1 name.

@alextnetto
Copy link
Author

Has this passed a DAO vote yet? The last time these changes were mentioned on the forums I asked this and was told it would still require a DAO vote.

https://www.ensgrants.xyz/rounds/1/proposals/16

Some other benefits are the gas savings in these new features as shown here

@Arachnid
Copy link
Member

Outstanding work! I'll give this a more detailed review soon, but I had one initial thought about the tips/referral mechanism that bears thinking about. What if, instead of maintaining a mapping to individual commits for pre-payment, we keep a per-account balance that can be used both to fund registrations and for referral fees? A commit-with-fee can fund this account, and referral fees can be added to the users' balance, while a reveal consumes any balance in the user's account, sending the remainder back to them so they don't have to manage withdrawals.

This will require keeping a separate counter of accumulated fees that can be withdrawn to the DAO, but I think it should still work out lower gas-cost than sending the referrer their fee in each TX, and simplifies the UX and the code.

@lcfr-eth
Copy link
Contributor

lcfr-eth commented Sep 20, 2022

edit: just saw this was posted to the forum stating it will need a full vote still - apologies!

https://discuss.ens.domains/t/ens-controller-v2-is-ready-for-code-review/14510

@jefflau
Copy link
Member

jefflau commented Sep 21, 2022

Awesome stuff!

There are a few changes we made post audit, so this will need to be merged into your V2 code as well. One example is:

https://github.com/ensdomains/ens-contracts/blob/master/contracts/ethregistrar/ETHRegistrarController.sol#L308

Please have a look at the current .eth controller contract for other additional changes. I will also find some time to take a closer look and give more detailed feedback

* @param _referralFee The fee to be used. From 0 to 1000.
*/
function setReferralFee(uint256 _referralFee) external onlyOwner {
require(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to use custom errors as opposed to text.

@alexvansande
Copy link

@Arachnid "What if, instead of maintaining a mapping to individual commits for pre-payment, we keep a per-account balance that can be used both to fund registrations and for referral fees?"

I agree. This is actually something I have disagreed on implementation with @alextnetto. Initially we had agreed to have a general balance for everything: fees, referrals, tips, funds, etc, but in the end the team argued that it was actually more costly in terms of gas to save the new balance than just to send the ether directly. As they were optimizing for gas I let it pass, but I'm still curious about why that would be the case.

@alextnetto
Copy link
Author

Outstanding work! I'll give this a more detailed review soon, but I had one initial thought about the tips/referral mechanism that bears thinking about. What if, instead of maintaining a mapping to individual commits for pre-payment, we keep a per-account balance that can be used both to fund registrations and for referral fees?

Here is why we send the ether directly

@Arachnid
Copy link
Member

Here is why we send the ether directly

Did you test this in the case where the accounts already have balances? Setting a storage slot from zero to nonzero costs 20k gas, but changing its value if it's already nonzero costs only 5k.

@alextnetto
Copy link
Author

Heyy friENS, what's missing for getting this for a voting on DAO, can I do something to help, review, modify?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants