Refactor on App struct

[zoruka]

Thinking about the extensibility of our contracts, I want to bring up the discussion about refactoring our contract to not use structs for storing the metadata. The goal with that is split our fixed struct that defines the token metadata in single mappings for each of these data fields we want to store of each NFA.

Change 1

Our current implementation over NFA metadata is:

struct App {
    string name; // Name of the site
    string description; // Description about the site
    string externalURL; // Site URL
    string ENS; // ENS ID
    uint256 currentBuild; // The current build number (Increments by one with each change, starts at zero)
    mapping(uint256 => Build) builds; // Mapping to build details for each build number
    string logo;
    uint24 color; // Color of the nft
    bool accessPointAutoApproval; // AP Auto Approval
}

mapping(uint256 => App) private _apps;

And as we are developing and needing to add more fields to store token data, we might change this struct to include more as we develop. This implementation make harder to split our logic between different contracts that could take care only about a new feature we want to implement. So my proposal is to change the implementation above granulating the data in small chunks that may be implemented by modular contracts and be extended by our main contract FleekERC721.

We could have:

// Implementing only the metadata fields and how to interact with them
abstract contract FleekMetadata {
    mapping(uint256 => string) private _tokenName;
    mapping(uint256 => string) private _tokenDescription;
    mapping(uint256 => string) private _tokenExternalURL;

   // All the read/write functions to treat these data
}

// Implementing only ENS data and how to interact with it (validation for instance)
abstract contract FleekENS {
    mapping(uint256 => string) private _tokenENS;

   // All the read/write functions to treat these data
}

// Implementing only the info about app branding
abstract contract FleekBranding {
    mapping(uint256 => uint24) private _tokenColor;
    mapping(uint256 => string) private _tokenLogo;
    // We might include fields like discord, twitter as they are needed

   // All the read/write functions to treat these data
}

contract FleekERC721 is FleekMetadata, FleekENS, FleekBranding {
    uint256 private _appIds; // as we already have

    function mint(args) public {
        uint256 id = _appIds;
        _tokenName[id] = name;
        _tokenDescription[id] = description;
       // and goes on...
    }

     function tokenURI(uint256 tokenId) public view returns (string memory) {
        // gets the datas stored and each module and format them in the way we want to display
    }
}

The implementation above would have three main benefits as far as I can see:

• We can add more fields on the run, as we are using proxy implementation and applying correctly the gap between our contracts (https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps);
• We may have optional fields easily that may set when the token owner want;
• We will have a better organization in the code where each contract could work as a module wrapping just the logic about it's feature

An example about this implementation can be seen on #151, where I did split out the access points feature to a single contract and removed the accessPointAutoApproval from App struct. And this also comes along with how ERC721 we extend works look here.

Change 2

Another point to think about is by changing those mappings from private to public. Making easy for interactions directly with a desired property. If we make:

mapping(uint256 => string) public tokenName;

other contracts and clients may get the info by:

contract.tokenName(0);

This change may also be applicable if we keep the same structure we have right now, but changing the app mapping to public, which seems a better approach then having this function.

The tradeoff of this implementation is that allows inherited contracts to change the values stored in these mappings without passing the logic that may be applied over these fields setting them.

All these changes would require time and are kind of critical at our stage, so we might get all feedback as possible before moving on and considerate this as a good approach and want to implement that.

[daltoncoder]

I agree with both changes. I think even if you dont go with change 2 these mappings would be better as internal(the default when not specified), or else you would need to make setter functions instead of doing _tokenName[id] = name in the mint and other places

[EmperorOrokuSaki]

Although using structs provides some benefits such as struct packing to save gas, I think in this case moving metadata fields away from the App struct is a good decision in terms of providing support for future metadata fields that we might add.

I also agree with the second proposed change.

[jsonsivar]

I also agree, this is a good step in modularizing our metadata and different features. I definitely want to start by saying this should be the next step in our contract iteration because it makes it modular and extensible without adding too much structure change and complexity (relative to what I mention below). I'm also on board with making the mappings public - as long as it's only the inherited contracts that could update it.

Below I'm sharing some thoughts as I was thinking about this, but we don't need to action on it.

---

As an aside, this discussion also made me re-visit the factory pattern idea so I was taking a little time thinking about that. It still needs further development and design so just posting for a thought exercise and maybe it'll spark other ideas. I know we discussed this earlier and there were some cons too. But I also think there are some pros like ability to own things and also create a standard to make anything on-chain an NFT without coupling the ERC721 code and the underlying business contracts.

Like if we had different interfaces for FleekBase and then FleekSite or FleekSubgraph would inherit from FleekBase. But instead of a mapping they would just be private variables. E.g.,

contract FleekBase {
    string private name;
    ...
}

contract FleekSite is FleekBase {
    string private externalUrl;
    ...
}

And then the mint would create a new instance of this Fleek interface and the apps mapping would be like mapping(uint256 => FleekBase). Ie.,

    function mint(
        address to,
        string memory name,
        ...
    ) public payable returns (uint256) {
        uint256 tokenId = _appIds;
        _mint(to, tokenId);

        _appIds += 1;

        FleekSites site = new FleekBase(...);

       _apps[tokenId] = address(FleekSites);
        return tokenId;
    }

Of course, there are lots of things to figure out with this approach. For example we'd need mints with different overrides for the different types. But the pros are that it's a clean polymorphic structure for the nfa business logic and also gives flexibility to attach any contract as the URI (similar to having an IPFS file as an external object). Another issue with this ownership. The owner on the new contracts need to be in sync with the owner on the NFT contract. Also not sure how storage slots are impacted by this design vs the metadata field mappings.

Basically something like this:

Also this is a bigger change and needs way more vetting so I will think about it but sharing the analysis.