delegatexyz / delegate-registry

172 stars 52 forks source link

Confusing Enumeration? #18

Closed ryley-o closed 1 year ago

ryley-o commented 1 year ago

Enumeration is currently slightly confusing (although it is well-described in the natspec comments).

Example:

That being said, the existing views might still be useful, and may be required to fully understand the state via enumeration, depending on which proposed solution is selected below.

Perhaps this could be made more intuitive if we add an additional view function named getAllDelegationsForToken, which could:

Cc @jakerockland

0xfoobar commented 1 year ago

@ryley-o I've been thinking through the optimal interface, now that we've cracked all desirable functionality (delegation, revocation, onchain enumeration for a vault, onchain enumeration for a hotwallet)

People will interact with this smart contract via three separate means:

When designing function interfaces it's important to look at function consumers and what matters most to them.

Current interface is:

Write Functions

I'm willing to require more complex parsing logic on the delegatecash website (since it only has to be written once, and has no dev adoption concerns) than I am on the nftwebsite and nftcontract (since this has to be done over and over again for each project which adopts the delegation registry.

We have two key functions there, getDelegationsByDelegate() and checkDelegateForTYPE(). On the nftwebsite side, making the return values of getDelegationsByDelegate easy to parse and understand is critical, while gas usage doesn't really matter since it's a read RPC call. On the nftcontract side, calls into checkDelegateForTYPE will use gas on every single mint, so it's important to minimize usage there.

This raises the question: what interface do we expect a consumer NFT mint contract to use? There are two options I would consider as a consumer: (1) Call checkDelegateForContract once on the specific collection I care about, if that succeeds we don't need to call it multiple times for multiple tokens. If it fails then fallback to checkDelegateForToken (which itself falls back to checkDelegateForContract, a bit of a waste (2) Call checkDelegateForToken once per token, this could be inefficient if the vault has approved at a collection or wallet level.

This is further complicated by the possibility of batch mints, one hotwallet minting for two different vaults that have both delegated to it, etc

ryley-o commented 1 year ago

@0xfoobar great summary. I tend to expect the checkDelegateForToken to be called by minting-like contracts. This is because "claimable-like" actions (airdrops, ability to mint, etc.) are typically allocated on a per-token basis to owners, not per wallet. Even if a delegate were claiming on behalf of multiple tokens at a single time, I would imagine integrating contracts would tend to call each token individually to avoid reentrancy-like stuff. FWIW, I don't think checking token-contract-all would add too much gas.

wwhchung commented 1 year ago

A consuming contract should probably take in a hint on which method to use, passed in via the frontend of the dapp, which can check cheaply.

ryley-o commented 1 year ago

Write Functions delegateForAll (used on delegatecash) delegateForContract delegateForToken revokeAll (used on delegatecash) revokeDelegate revokeSelf Read Functions getDelegationsByDelegate (used on nftwebsite) checkDelegateForAll (used on nftcontract) checkDelegateForContract checkDelegateForToken getDelegationsForAll (used on delegatecash) getDelegationsForContract getDelegationsForToken

Any appetite to include enumeration of "on which contracts / which tokens does my vault currently have active delegations?" Currently, users must query each contract or token of interest via getDelegationsForContract/getDelegationsForToken. We could add capability to enumerate getContractsWithActiveDelegations/getTokensWithActiveDelegations, which would enable users to fully understand their current state without off-chain indexing. At a basic level, user could visit "delegatecash" and immediately understand all delegations, including contract and token delegations (which we currently can't enumerate)

Thoughts @0xfoobar? I could take a crack at this if you think it might be worth the gas.

0xfoobar commented 1 year ago

That would be useful @ryley-o

ryley-o commented 1 year ago

closing this due to merge of #25 👍