Fungible Token Switchboard contract and tests

[alilloig]

Closes: #66

Description

Created FungibleTokenSwitchboard contract. Created transactions for setting up switchboard and manage them. Created test suite using js. Design proposal can be found here

---

• [x] Targeted PR against master branch
• [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• [x] Code follows the standards mentioned here.
• [x] Updated relevant documentation
• [x] Re-reviewed Files changed in the Github PR explorer
• [x] Added appropriate labels

[joshuahannan]

Can you put the test directory into a lib/js/ directory so it isn't at the top level? Thank you!

[joshuahannan]

Also, it looks like you haven't pulled the latest changes from master to merge these changes into your branch. Can you do a git pull and push your branch also to make sure that only your changes show up? Thanks!

[bluesign]

This is great contract by functionality, but I think terrible UX.

I suggest to improve:

• Let's get rid of capability part ( I mean user facing part ) RemoveVault can take a Type which is more natural, AddVault can actually take a Path. ( We can leave the advanced usage case as AddVaultCapability, so advanced users can add vaults in other accounts etc )

• Let's add createSwitchBoardFromPaths( [PublicPath] ): @SwitchBoard helper function. So I can just initialize this contract with a single line of code.

• Later when storage enumeration lands, we can even auto define Public Receivers on init. ( whatever FungibleToken they are ) So if I have Flow, FUSD, USDC they will be all automatically defined.

I also have very strong feelings on panic on deposit. But don't have a good suggestion yet.

[joshuahannan]

@bluesign I like your idea to use Path to add the vault and initialize the switchboard. I think it would be good to include that. createSwitchBoardFromPaths( [PublicPath] ): @SwitchBoard would also need an Address argument, right? so it knows which account to get it from?

I don't agree about using Type instead. That would require more lines of code in the transaction, whereas right now you can just get your receiver capability from your account and just pass that in directly instead of having to get the type also.

[bluesign]

@joshuahannan yeah you are right, Address is needed there.

Or we can make it: AddVaults( [PublicPath] ), AddVault( PublicPath ) ( which we can call after we save the switchboard to storage, so we can eliminate the address part ) we can panic with pre conditions with self.owner=nil

For Type part: it is little tricky for me:

pub fun removeVault(capability: Capability<&{FungibleToken.Receiver}>)

here when I look at the signature, I see it will remove some capability, but actually it is removing some Type ( as key of the dictionary ) But I don't have strong feelings for this one. I am a bit torn too.

[alilloig]

For the type discussion I'm still aligned with the original implementation, I did it with the tx in mind, I believe it's the simpler way of storing it into the switchboard as josh said. Keep in mind @bluesign that we are actually deleting the dictionary entry that is storing the capability that you are passing as an argument, we are just using the Type of the reference that can be borrowed from that capability as the key for indexing that.

And relating to add new capabilities using paths, I also like the idea, but I believe we should pass an address also as an argument. The other option will be to check sor resource.owner, and even if the Switchboard resource probably is extremely unlikely to abandon their owner's account, it still feels a little like checking msg.sender, and that could be just avoided passing the Address parameter. Also we will be adding the feature of adding several vaults with just one call to the resource which I think is quite nice. What do you think of something like this?

/// addNewVaultsByPath adds a number of new fungible token receiver 
///                    capabilities by using the paths where they are
///                    stored
///
/// Parameters: paths: The paths where the public capabilities are stored
///             address: The address of the owner of the capabilities
///
pub fun addNewVaultsByPath(paths: [PublicPath], address: Address) {

[bluesign]

pub fun addNewVaultsByPath(paths: [PublicPath], address: Address)

this looks ok to me, just commenting something small on implementation.

[alilloig]

Something pointed by @bluesign that I believe it's worth giving a thought. If a user tries to store a capability for a vault that has already been stored, what the switchboard should do? Now is just overwriting the capability, but it will also make sense to not allow users to overwrite them but rather have them delete it first if they want to add a different capability for the same FT. What do you think @satyamakgec @joshuahannan ?

[joshuahannan]

I think requiring them to delete it first before overwriting is a good idea

[alilloig]

I think requiring them to delete it first before overwriting is a good idea

Great, if it makes sense I'll make the addNewVault() panicking if there's already a vault for that type, while keeping addNewVaultsByPath() panic free

[alilloig]

What do you think of an event that would be emitted if a deposit fails?

    /// NotCompletedDeposit
    /// 
    /// The event that is emitted when a deposit can not be completed
    ///
    pub event NotCompletedDeposit(type: Type, amount: UFix64, 
                                    switchboardOwner: Address)

        /// deposit Takes a fungible token vault and routes it to the proper
        ///         fungible token receiver capability for depositing it
        /// 
        /// Parameters: from: The deposited fungible token vault resource
        ///        
        pub fun deposit(from: @FungibleToken.Vault) {
            if let depositedVaultCapability = self.receiverCapabilities[from.getType()]{
                if let vaultRef = depositedVaultCapability.borrow() {
                    vaultRef.deposit(from: <- from)
                } else {
                    emit NotCompletedDeposit(type: from.getType(), 
                                        amount: from.balance, 
                                        switchboardOwner: self.owner!.address)
                    panic ("Can not borrow a reference to the the vault")
                }
            } else {
                emit NotCompletedDeposit(type: from.getType(), 
                                        amount: from.balance, 
                                        switchboardOwner: self.owner!.address)
                panic ("The deposited vault is not available on this switchboard")
            }
        }


        /// safeDeposit Takes a fungible token vault and tries to route it to 
        ///             the proper fungible token receiver capability for 
        ///             depositing the funds, avoiding panicking if the vault is 
        ///             not available
        ///             
        /// Parameters: vaultType: The type of the ft vault that wants to be 
        ///                        deposited
        ///
        /// Returns: The deposited fungible token vault resource, without the
        ///          funds if the deposit was succesful, or still containing the
        ///          funds if the reference to the needed vault was not found
        ///
        pub fun safeDeposit(from: @FungibleToken.Vault): @FungibleToken.Vault? {
            // Try to get the proper vault capability from the switchboard
            // If the desired vault is present on the switchboard...
            if let depositedVaultCapability = self.receiverCapabilities[from.getType()] {
                // We try to borrow a reference to the vault from the capability
                // If we can borrow a reference to the vault...
                if let vaultRef =  depositedVaultCapability.borrow(){
                    // We deposit the funds on said vault
                    vaultRef.deposit(from: <- from)
                    return nil
                } else {                    
                    // If we can't borrow a reference to the vault we emit the
                    // proper event and return the passed vault so the depositer 
                    // can recover their funds
                    emit NotCompletedDeposit(type: from.getType(), 
                                        amount: from.balance, 
                                        switchboardOwner: self.owner!.address)

                    return <- from
                }
            } else {
                // If the vault was not found on the switchboard and the deposit
                // could not be completed we emit the proper event and return
                // the passed vault so the depositer can recover their funds
                emit NotCompletedDeposit(type: from.getType(), 
                                        amount: from.balance, 
                                        switchboardOwner: self.owner!.address)              
                return <- from
            }
        }

[joshuahannan]

If the execution reverts, then the events aren't emitted I believe so it wouldn't make sense for the regular deposit. It would probably be good for the safe deposit though

[bluesign]

great idea. Just code is little too complex now to grasp at first sight. maybe something like this?

 pub fun safeDeposit(from: @FungibleToken.Vault): @FungibleToken.Vault? {
            // Try to get the proper vault capability from the switchboard
            // If the desired vault is present on the switchboard...
            if let depositedVaultCapability = self.receiverCapabilities[from.getType()] {
                // We try to borrow a reference to the vault from the capability
                // If we can borrow a reference to the vault...
                if let vaultRef =  depositedVaultCapability.borrow(){
                    // We deposit the funds on said vault
                    vaultRef.deposit(from: <- from.withdraw(amount: from.balance))
                }
            }
            
            // if deposit failed for some reason 
            if from.balance>0{
                emit NotCompletedDeposit(type: from.getType(), 
                                        amount: from.balance, 
                                        switchboardOwner: self.owner!.address)              
                return <- from
            }
            
            destroy from 
            return nil
}

[alilloig]

If the execution reverts, then the events aren't emitted I believe so it wouldn't make sense for the regular deposit. It would probably be good for the safe deposit though

thank you, I wasn't 100% sure if the events emitted before the execution was aborted were emitted anyways

[alilloig]

@bluesign I liked your way, event added!

[alilloig]

And I came up with a thought, should not the getVaultTypes() method check if all the stored capabilities can be borrowed, and if any of them cannot be borrowed, don't return that Type? So the caller can be truly aware of which FT can actually deposit into the switchboard

something like

        pub fun getVaultTypes(): [Type] {
            let efectitveTypes: [Type] = []
            for vaultType in self.receiverCapabilities.keys{
                if self.receiverCapabilities[vaultType]!.check<>(){
                    efectitveTypes.append(vaultType)
                }
            }
            return efectitveTypes
        }

[alilloig]

as soon as the new version of the flow-js-testing is released on npm (it will add support for Paths) I'll update it and request a re-review from you guys

[joshuahannan]

@alilloig Can you pull the latest from master and fix the merge conflicts that are shown here? Thanks!

[alilloig]

@joshuahannan I believe is ready to merge!