Authorized deployer contract PRO-80 #39
Conversation
PRO-80 Deployer contract for deterministic deployment addresses
We wish for all deployments of V5 to be at an address that is known in advanced. Using a Deployment contract, anyone determine V5 was deployed by 0x, if 0x is the owner of that Deployment contract. Follow Similar patterns to the |
…or integrators to `eth_call` to get most recent non-buggy instance
… correct fee collector address
duncancmt
force-pushed
the
deployer
branch
2 times, most recently
from
72a95bf to
f1f7d85
Compare
| revert FeatureInitialized(feature); | ||
| } | ||
| content = string.concat( | ||
| "{\"description\": \"", description, "\", \"name\": \"0xV5 feature ", ItoA.itoa(feature), "\"}\n" |
There was a problem hiding this comment.
I think it might be useful to keep the possibility of updating the metadata here. especially since a feature can be deployed many times.
Also it might have some value having an image field, so as the NFTs can (potentially) have some visual representation when viewing them. (Nice to have ofc)
There was a problem hiding this comment.
here's how I'm thinking about this, but we should continue the conversation about how to represent feature metadata.
features ought to have stable ABIs (or at least, backwards-compatible ABIs) and consequently usage/documentation. stability as a base layer for settlement is a major selling point of 0x Protocol and 0x API. therefore, if something warrants the modification of description, it should instead result in the creation of a new feature with brand-new metadata. this makes protocol upgrades opt-in for existing integrators.
putting images in the NFT metadata adds a whole extra layer of complexity and attack surface. first of all, not all NFT marketplaces support inline images in NFT metadata, so we probably have to add an indirect URI, which would also need to be computed on-chain and uploaded into IPFS separately. we're already going to struggle with IPFS CID versioning[0] as-is, so this just compounds the problem. then we have the additional attack surface involved in image decoding. of course, we've already got attack vectors in description involving malformed Markdown or badly-escaped JSON, but at least that's likely to result in brokenness rather than code execution.
do you feel like there's significant value to developers to add image metadata to 0xV5 NFTs? enough that warrants the additional headache and gas?
[0]: this contract only supports CIDv0; supporting CIDv1 would be good to implement, but is presently more work than I want to put into this
There was a problem hiding this comment.
This absolutely makes sense, and I agree with the stable ABI condition. But still if the feature were to have a new call it may need updating the description, but maybe that's not the role of the description here. And we should maybe define what level of details goes in there.
For the image, yes I was referring to the standard way atm, using an image url, where image is stored on IPFS. So this would be a nice to have from a community/comm pov. And I don't think it has an importance for devs/integrators. The attacks mentioned if I understand well are not specific to this implem, and are in fact valid of most of existing NFTs I think
I guess my concern is more about how to keep possibilities open for updating/upgrading the metadata, and less about adding things now
There was a problem hiding this comment.
Well, the whole Deployer contract can be made upgradeable (see #56 ).
Yes, you're right that the attacks I mention exist for pretty much all NFTs. Perhaps it's elitist of me, but I like to think that 0x holds ourselves to a higher standard of security than most DeFi projects (see our response to the Socket/Bungee hack as an example of the dividends paid by this approach). I would feel better about integrating images into these NFTs if we had better tooling for putting that data into IPFS in a way that isn't going to make us want to pull our hair out over the proliferation of subtly incompatible encoding standards. CIDv1 would be a good start to that.
The PermanentURI event prevents the metadata from changing, even in the face of upgrades, but it could be made mutable for future iterations by omitting that event. As far as descriptions, let me confer with @dekz and @wonge97 to get you an example.
There was a problem hiding this comment.
Sorry jumping in here late, catching up with the conversation. From my POV, I agree with Duncan that the description in the metadata does not need to be updateable. The reason for this being that I would encourage the description to be fairly high level while the exact details and minor changes are captured in documentation elsewhere.
There was a problem hiding this comment.
is there a working schema for the metadata? wasn't able to find one by searching
There was a problem hiding this comment.
OpenSea has a pretty good explainer: https://docs.opensea.io/docs/metadata-standards
The ERC721 standard itself also explains it a little, but OpenSea is better https://eips.ethereum.org/EIPS/eip-721
There was a problem hiding this comment.
ok, i thought there might be some specific expectations/requirements for settler features, but it sounds like there aren't (at least not yet)
There was a problem hiding this comment.
The description field of the NFT metadata is intended to be a brief, human-readable, developer-oriented description of the functionality/featureset of the specific 0xV5 deployment. Basically, if somebody is poking around on a blockchain explorer, it should give them an idea of what they're looking at without resorting to reading the source code. It's not intended to replace 0x API documentation nor natspec comments nor any long-form documentation in this repo.
No description provided.