Proposal: Updaing the D1 Fund XToken Deployment Process

Product
1

Authors

  • Owen
  • w/ assistance from finesseboi

Summary

Hey all,

I’d like to open the discussion for changing the way that XToken deployments are currently done for the D1 funds.

Some context for people who may be unfamiliar: The current D1 fund creation process works is essentially done in 2 steps. In step one, users deploy an XToken with their desired name and symbol. In step two, they register this token with the NFTX master contract, along with the ERC-721 token they would like it to be associated with.

Currently, on the NFTX front-end, users are actually deploying the entire bytecode for an entirely new XToken each time they want to make a new D1 fund. This can be quite costly, 0.06 to 0.07 ETH depending on gas prices.

Effects

Because every XToken has the same functionality, we can save future users a substantial amount of gas by instead using the minimal proxy pattern. This has one “master contract” keep track of the actual logic for the implementation, and each child contract can store just the data like balances, name, symbol, etc.

Specifications

I’ve gone ahead and coded up an implementation of this for the XToken contracts that we currently make users deploy. The code (along with tests) can be found on GitHub here.

Because NFTX is an open permissioned system, I could just deploy this code onto mainnet and allow people to use it as an alternative to the front-end. However, I think the cost savings are substantial (around 40% when I tested), which is why I think it’s worth integrating directly in the front-end for users to use. Thus, I’m making this proposal to get more eyes on the code and to talk about integration in a more official capacity, and I’m holding off on deploying to mainnet for right now.

Additionally, if we use the factory code to create XTokens, it also helps avoid a potential issue in the NFTX contracts. (Already reported to Alex.)

Next steps

  • Play with the current version on Rinkeby here by calling createXToken.

  • Getting people’s opinions

  • Getting some more eyes on the code

  • Open discussion about more formal integration with the front-end

  • Technically, if we decide to approve this change, what would happen is:

    • I deploy the new contracts, and I verify them on Etherscan.
    • I set the NFTX proxy to be the owner, so it matches the setup of all the other XTokens.
    • We add the XTokenFactory contract info/ABI to the front-end, and change it so that when users deploy a D1 fund, they first make a call to the XTokenFactory, and upon success, we call createVault with the newly deployed XToken contract’s address. This is almost identical (afaik) to the current user flow, with the only change being the the first step. (Instead of manual user deployment, they now make a cheaper function call.)

At a later point, there are also a few core NFTX contract changes we can make which take advantage of the adoption of the XTokenFactory, but I will save those for a separate proposal if this one passes.

Funding Request -

Yes - At minimum, I would like the gas fees for contract deployment to be refunded. Additionally, if the proposal passes and we integrate this version into the front-end, I’d also ask for an NFTX token bounty because this upgrade both patches a security hole and improves gas costs for users.

We don’t have a security disclosure policy set up yet, and after talking with Alex, the attack vector I noticed was similar to something already covered in the audit. I think it’s best to keep this discussion about whether or not to implement these changes, and save the amount discussion for when we talk about incentivizing contributors and security disclosures.

Communication

Quorum and Governance Notes

  • Minimum Quorum: As this is only a draft, no quorum is required.

  • Passing Threshold: As this is only a draft, no quorum is required.

3 Likes
2

My only concern here is having people review the new contracts. I also would agree with gas reimbursements and a fair (relative) bounty.

If anyone could review the contracts more that would help us push this forward and implement as you are pretty much ready to go.

2 Likes
3

First off, this is great work @0_0! Not gunna lie, I did not know about this pattern.

I think you should 100% be reimbursed for any gas spent researching and developing this. Retroactive rewards for your time are also something we should consider later.

As I was reading this I was thinking we should start on this migration this week, since it makes sense to do it sooner rather than later, however then I started thinking about whether we should maybe wait to do a future migration that has many features which we want, so that we do not migrate multiple times over, say, a six month period.

For instance, one feature we will want to add ASAP is the ability to create fund “rules” which say, for example, “any token ID’s less than 500 are eligible” (instead of manually adding 1,2,3,…,498,499,500). In other words, there are many inefficiencies in the current contracts (mostly due to my solidity noobishness) and it may make sense for us to batch many upgrades into a single version 2 migration.

If we decide to do that, then all of this can still be implemented, we would just hold off on deploying it soon. What do you think?

4

True, I think those are all useful things we can add to a future version. (Also I actually had a separate idea about the interval rules, we can jam on that later.)

I think it’s worth deploying this fix in particular earlier rather than later because it doesn’t affect any other part of the NFTX contracts so it’s not like we’d be doing a new deployment of any of the core contracts. It’s just a change in the front-end (to call the function instead of deploying bytecode), and it cuts gas in half.

Granted, fixing the security loophole (i.e. adding require(isCloned(addressOfXToken) requires an update to the NFTX contracts, but even that aside, I think it’s worth pushing this sooner rather than later because of the immediate gas savings.

If we assume that we’ll see 20 more D1 funds in the next month or so, this is ~$600 in combined savings, which is def less than the costs of deployment.

2 Likes
5

Agree with everything here! I’ll try to get to this next week. Feel free to add it to the Notion board if you want in the meantime but no pressure if you are busy.