flow icon indicating copy to clipboard operation
flow copied to clipboard

FLIP: Cadence borrowContract - Allow Dynamic Import

Open bluesign opened this issue 2 years ago • 15 comments

FLIP for new publicAccount method to Cadence to borrow contracts dynamically.


For contributor use:

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

bluesign avatar Jul 25 '22 20:07 bluesign

@bluesign is attempting to deploy a commit to the Flow Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Jul 25 '22 20:07 vercel[bot]

it allows us to create a contract dependency graph, for example, which we can use to optimize things like program caches to improve performance

I don't have much info about program caches, but currently seems it is just resetting cache on each contract update.

But I think program cache argument is a good one. Do you have any idea how will it look when contract dependency graph is used?

I mean currently I can borrow an object without importing its contract and use the methods of its interface, anything different here ?

Would it be reasonable to restrict the string argument to borrowContract to being a string literal so we can still do static analysis? Or would that defeat the point of the feature?

I think this defeats the point.

bluesign avatar Jul 26 '22 15:07 bluesign

But I think program cache argument is a good one. Do you have any idea how will it look when contract dependency graph is used?

Yeah. Currently the program cache is completely wiped on a contract update, because we don't know which downstream contracts might be affected by this update. If we have a dependency graph though, once update we are considering is to use this information to only invalidate cached contracts that depend (transitively) on the updated contract; which should dramatically decrease the amount of unnecessary cache invalidation that we do.

A solution to https://github.com/onflow/cadence/issues/1684 would also likely require a dependency graph, as the way to avoid double counting interactions during imports would require us to know "where" interactions came from when they are imported.

These are just the examples that I know of off the top of my head, but I believe @SupunS and @ramtinms have been thinking about this as well.

I think this defeats the point.

I think this would still potentially address the issue where one broken contract among all the imports would cause them all to fail, so there might still be some benefit. Additionally I think you could do limited dynamic importing via something like:

if condition {
   contract = borrowContract("myContract")
} else {
   contract = borrowContract("myOtherContract")
}

dsainati1 avatar Jul 26 '22 15:07 dsainati1

If we have a dependency graph though

@dsainati1 I think you mean dependency graph of all contracts on chain, not the contract got updated right?

bluesign avatar Jul 26 '22 16:07 bluesign

@dsainati1 I think you mean dependency graph of all contracts on chain, not the contract got updated right?

This is an implementation detail I think. We would only need to know which contracts are downstream of the one that was updated, whether this is done on demand on each update or whether we have a persistent graph is an implementation detail.

dsainati1 avatar Jul 26 '22 16:07 dsainati1

We would only need to know which contracts are downstream of the one that was updated

So we only care about the descendants of the contract. ( contracts that are importing mine )

Let's call this example contract: 0xMAIN.Test, if I write below code:

import NonFungibleToken from 0xSOMEWHERE 
pub fun someFunction(){
  if let contract = getAccount(0x1).borrowContract<&AnyResource{NonFungibleToken}>("ExampleNFT") {
 	  let emptyCollection <- contract.createEmptyCollection()
   }
}

That means:

  • 0x1.ExampleNFT imports 0xSOMEWHERE.NonFungibleToken
  • 0xMAIN.test imports 0xSOMEWHERE.NonFungibleToken

Let's say we have 3 cache slots:

  • 0x1.ExampleNFT
  • 0xSOMEWHERE.NonFungibleToken
  • 0xMAIN.test

When a transaction execute:

import test  from 0xMAIN
transaction{
  execute(){
    test.someFunction()
  }
}

Now order of business here :

  • getProgram(0xMAIN.test) //cached. ( I think which also includes NonFungibleToken here )
  • getProgram(0x1.ExampleNFT) //cached

this would not be no different than:

import NonFungibleToken from 0xSOMEWHERE 
pub fun someFunction(@NonFungibleToken.NFT){
  //where NonFungibleToken.NFT is type 0x1.ExampleNFT.NFT
}

@dsainati1 I don't know much about internals, can you show me what I am missing here ?

Main question is why 0xMAIN.test should be invalidated when 0x1.ExampleNFT changed?

bluesign avatar Jul 26 '22 16:07 bluesign

I'm yet to fully go through the FLIP. But as for the program cache problem, I think statically knowing the dependency graph is not always required. (Feels like going a bit off topic here, maybe we should move that to a separate discussion/thread).

For example, a static dependency graph is only required if we are going to do an 'active' cache invalidation (i.e: invalidate cache at the time of deployment). But for a 'passive' invalidation, it is possible to invalidate (re-parse/re-check) a contract only when that particular contract is used by someone.

e.g: consider the scenario where A imports B. And B is updated.

  • Active invalidation:
    • Option I: Wipe the entire cache (This is how it happens at the moment)
    • Option II: Clear A's cache (and only A) when B is updated. (This is the future improvement @dsainati1 is pointing out that would require knowing who is importing who. I guess the argument there is, if A had dynamically imported B, then this option-II is not feasible)
  • Passive/on-demand invalidation:
    • Every contract in the cache can have a hash/uuid. Updating the contract changes this hash/uuid.
    • When A imports B, A keeps the hash/uuid of the imported B.
    • When B is updated, then B's hash/uuid changes. No changes done to A (or A's cache)
    • When someone imports A, check A's imports for any hash changes (compare the one in-hand with the B's actual hash in the cache). If there are discrepancies, then invalidate A.
    • This approach does not need to know the dependents.

I'm not really sure about the other problem (https://github.com/onflow/cadence/issues/1684) though, but it might be possible to do something similar there as well.

SupunS avatar Jul 26 '22 21:07 SupunS

@SupunS Very good example with passive invalidation, thanks.

Btw I still think programs should be cached separately and linked at runtime. And program cache should be something bounded like LRU cache.

But what I want to point out here, in our proposal, it is nothing that prevents cache. And it is no different than getting an object from storage ( which you haven't important their contract ) and operating on the interface. ( think this transaction as a function of a contract for example: https://github.com/onflow/flow-ft/blob/master/transactions/generic_transfer.cdc )

Maybe I am missing something here.

bluesign avatar Jul 27 '22 07:07 bluesign

@bluesign Yeah, I do agree, caching should be a separate problem (or shouldn't be a problem). i.e: internal details like caching shouldn't prevent Cadence language features. @dsainati1 also has a point, that there may be implementation complexities. But again, I think this whole discussion should happen elsewhere, perhaps a separate Cadence git issue.

I would very much like to get the focus back on the core idea/proposal of the FLIP.

SupunS avatar Jul 27 '22 14:07 SupunS

@dsainati1 @SupunS To prevent this kind of contracts is sufficient benefit alone for me:

https://flowscan.org/account/0xeb8cb4c3157d5dac

bluesign avatar Jul 28 '22 15:07 bluesign

@dsainati1 @SupunS To prevent this kind of contracts is sufficient benefit alone for me:

https://flowscan.org/account/0xeb8cb4c3157d5dac

This very contract is the reason I started asking @bluesign about this issue. Over time these kinds of setups get so complex or import so much that they cannot even function anymore. If instead you could borrow a contract with an interface, Alchemy could have had contracts which wish to be supported implement that interface and then you would just borrow that contract with the Alchemy interface and call the same method on all of them.

One big issue that has happened with this setup in the past, and which @bluesign has flagged, is that a bad update to one contract bricks the whole thing because of its imports. When the June 15th secure cadence spork came around, it took a while to get this these contracts operational again.

But it isn't just about when cadence has breaking changes, theoretically this setup means any single contract in the import lists of these contracts can hold the others hostage by making their contract invalid somehow until maintainers can update the contract. Dynamic imports would not only make this kind of contract simpler, it would insulate them all from each other.

austinkline avatar Jul 28 '22 16:07 austinkline

Just read through the comments.

Cadence intentionally includes features that enable interoperability (e.g. dynamic dispatch, interfaces, etc.), which compromise the ability to statically analyze programs. The proposed feature follows that direction.

We dropped the early idea of requiring transactions to statically specify all accounts which will be read from/written to upfront, because it hinders interoperability in an organic ecosystem. The same point can be made about requiring programs to statically specify all contracts they will interact with during execution.

turbolent avatar Aug 16 '22 19:08 turbolent

@turbolent I implemented this ( I hope ), should I make a PR for it and then go over FLIP ? or go over FLIP and then make the PR ?

bluesign avatar Aug 31 '22 10:08 bluesign

@bluesign Awesome! 👏 🏅

Please feel free to open a PR for the implementation in the Cadence repo and then link to it here. Having an implementation for it greatly helps with evaluating the proposal (e.g. it demonstrates that what is being proposed can be implemented, and allows contributors and developers to try the feature and provide feedback).

turbolent avatar Aug 31 '22 17:08 turbolent

I am pretty noob in this area but tried to make something working. I may have some huge misunderstanding or mistakes there. https://github.com/onflow/cadence/pull/1934

bluesign avatar Sep 05 '22 14:09 bluesign

Over time, it's starting to seem to me that the best equivalent to a "contract" in Solidity isn't a contract in Flow, but instead it is "Type".

The specific motivating example used in this example talks about creating empty Vaults or empty Collections, but I think that an even bigger problem in Cadence is that you might want to reference Types from contracts you haven't imported, and I don't believe this proposal solves that problem. (Imagine a generic transaction template for buying an NFT. That template should have a mechanism for checking the type and amount of the purchase price, and the type and ID of the NFT. To do that strictly with native Type objects would require that the import statements are dynamic, or that you import all possible FT/NFT types!)

Personally, I would rather see us creating a mechanism to reference a Type without an explicit import, and then have empty Vault creation be a static method on the Type object, instead of a method on the Contract object.

Java has a robust notion of referencing types without imports, the following two snippets are equivalent and equally valid:

import java.lang.Math

int big = Math.max(5, 10)
int big = java.lang.Math.max(5, 10)

Perhaps in Cadence we should have something like this:

import FungibleToken from 0xf233dcee88fe0abe

let ftType: Type<&FungibleToken> = Type<&FlowToken.Vault from 0x1654653399040a61>()

let emptyV = ftType.createEmptyVault()

dete avatar Nov 04 '22 23:11 dete

@dete I really liked this; much more useful. Also, it is extending the FLIP instead of limiting it.

Pros:

  • We can move createEmptyVault and createEmptyCollection to Vault and NFT ( as static methods ) with upcoming NFTv2 and FTv2 this can solve a lot of problems.
  • I think we can also allow: let ftType: Type<&FungibleToken from 0xf233dcee88fe0abe>

Cons:

  • We need a way to implement static methods/fields (the static keyword can solve those ) But it requires updates to contracts, to make it backwards compatible need to set the contract method/fields default to static probably.
  • Probably we need shortcut methods for CompositeType and ReferenceType combined, for dynamic Type runtime generation. or do we need reference type here in the syntax?, maybe something like:
let ftType: Type<@FungibleToken.Vault> = Type<@FlowToken.Vault from 0x1654653399040a61>()

bluesign avatar Nov 05 '22 13:11 bluesign

I am starting documentation asap.

bluesign avatar Nov 16 '22 16:11 bluesign