implement hybrid custody #44
Conversation
| return address | ||
| } | ||
|
|
||
| priv fun parseUInt64(_ input: AnyStruct): UInt64?{ |
There was a problem hiding this comment.
There is no code comments in the whole contract, It would be good to have to increase readability.
There was a problem hiding this comment.
Sorry, I should have made it clearer that these are dependencies. The contracts in subdirectories (flow-utils, hybrid-custody, standards, utility) are all dependencies. Goes for a number of comments made below as well.
|
|
||
| /// Private interface for Capability retrieval | ||
| /// | ||
| pub resource interface GetterPrivate { |
There was a problem hiding this comment.
I don't know whether this contract is going to be used as part of the core contract for hybrid custody or not but I don't see the GetterPrivate name doing justice to a resource interface. I can't tell what it is going to do with its name.
Maybe better to rename it to AccessPrivateCapability or RetrievePrivateCapability.
There was a problem hiding this comment.
For HC-related naming conventions, could you make an issue if you feel strongly enough about the naming conventions? We've gotten the contracts deployed to Testnet and are shooting for mid-August Mainnet deployment, so we'd want to get any non-upgradable changes discussed and implemented in those contracts in ASAP.
|
|
||
| /// Exposes public Capability retrieval | ||
| /// | ||
| pub resource interface GetterPublic { |
There was a problem hiding this comment.
Same for this better to rename to AccessPublicCapability
There was a problem hiding this comment.
Same as above
| /// Capabilities is critical to the use case of Hybrid Custody. It's advised to use Factories sparingly and only for | ||
| /// cases where Capabilities must be castable by the caller. | ||
| /// | ||
| pub contract CapabilityFactory { |
There was a problem hiding this comment.
I think the contract name is confusing, In general factory represents a warehouse where something get created and in the sense of CapabilityFactory it means capabilities are going to be created but it seems the contract functionality is different.
I think the better name is CapabilityFactoryManager or CapabilityFactoryRegistry, Which means a contract which manages the different factories.
There was a problem hiding this comment.
The resource that holds Factory implementations is named Manager, so full name CapabilityFactory.Manager which is how it would be named outside of the contract. Do you think that naming convention provides full enough context?
| /// | ||
| /// @param type: The type to add to the allowed types mapping | ||
| /// | ||
| pub fun addType(_ type: Type) { |
There was a problem hiding this comment.
What restrict someone to add the same type in the deny list and that is already existed in the allowlist ?
There was a problem hiding this comment.
In this case, addType() is effectively an upsert, where the given Type is added if it doesn't exist and replaced if it does.
| /// @param cap: Capability to add | ||
| /// @param isPublic: Whether the Capability should be public or private | ||
| /// | ||
| pub fun addCapability(cap: Capability, isPublic: Bool) { |
There was a problem hiding this comment.
I don't know how feasible it is to have the function design like this, It is not consistent with the other functions as other functions are designed specifically for the public/private capabilities while this function differentiates the capability on the basis of the param i.e. isPublic instead of the function name itself like others
| pub fun main(addr: Address): AnyStruct { | ||
| let manager = getAuthAccount(addr).borrow<&HybridCustody.Manager>(from: HybridCustody.ManagerStoragePath) ?? panic ("manager does not exist") | ||
|
|
||
| var typeIdsWithProvider = {} as {Address: [String]} |
There was a problem hiding this comment.
| var typeIdsWithProvider = {} as {Address: [String]} | |
| var typeIdsWithProvider: {Address: [String]} = {} |
Maybe this way it is more cleaner
| var typeIdsWithProvider = {} as {Address: [String]} | ||
|
|
||
| // Address -> nft UUID -> Display | ||
| var nftViews = {} as {Address: {UInt64: MetadataViews.Display}} |
Closes: #41 #43 #46
Related: #43
Description
Replaces old
LinkedAccountswithHybridCustodyas hybrid custody solution. Also refactored tests using Cadence testing framework. JS tests have been replaced in favor of Cadence. README will be updated in a fast follow up.All new contracts are dependencies from the hybrid-custody source repo or standard contracts required locally for VS Code extension support.
For contributor use:
masterbranchFiles changedin the Github PR explorer