LSP: Contracts can be analysed multiple times

[hugocaillard]

It's possible to declare multiple contracts identifiers with the same source file in the Clarinet.toml

[contracts.counter-1]
path = "contracts/counter.clar"

[contracts.counter-2]
path = "contracts/counter.clar"

In this case, the "manifest.contracts" will have twice the same source code, and we'll compute the analysis twice. https://github.com/hirosystems/clarinet/blob/ed323ec4edbb70647fec5a0040b83d0c95ca81d3/components/clarinet-deployments/src/lib.rs#L497

It leads to some performances issues, for instance, here the .get() should be replaces with .remove() to avoid .clone()

Potential solutions

• emit an error if a path is used multiple times in a manifest
• rework the iteration to avoid analyzing twice the same source

[hugocaillard]

I've spotted this issue by testing the extension on CityCoins. It would be interesting to understand the specific use case and the need to have multiple identifiers for the same contract (cf here for instance)

cc @whoabuddy

[whoabuddy]

Hey @hugocaillard happy to add some color there.

The use case in that scenario was to create duplicate contracts in order to simulate an upgrade in the unit tests, where the initial contracts would be deployed and activated, then the core contract upgraded from core-v1, core-v1-1, core-v1-2 then running test scenarios run afterward.

Here is an example of one of the tests that goes through the core contract upgrade process using those: https://github.com/citycoins/contracts/blob/develop/tests/cities/mia/upgrade/miamicoin-core-v1-v2.test.ts#L36

This is going to be simplified in future iterations and the protocol is being restructured in citycoins/protocol, so we can avoid that pattern if it's a problem.

[mijoco-btc]

Another example - again for City Coins - say there are 1000 cities all with a separate vault for each treasury but the same underlying contract (the same contract deployed 1000 times with different names). Support for this in Clarinet is exactly as in the example above..

[contracts.treasury-city1] path = "contracts/treasury.clar"

[contracts.treasury-city2] path = "contracts/treasury.clar"

is a legitimate requirement for declaring multiple contract identifiers with the same source file in the Clarinet.toml ?

[whoabuddy]

Looks like something happened between 1.0.6 and 1.1.0 that causes contracts with this type of definition to fail clarinet check. See this comment for more information, and I confirmed it locally after upgrading as well.

I didn't see anything in the changelog - and per the comments above this is something we could actually use as some contract structures will be a template with multiple deployments.

Is there an official answer on how this should be handled, especially now that our flow is broken?

[lgalabru]

hey @whoabuddy! thanks for raising this issue. the pattern one source / multiple contracts was supported in prior versions of clarinet, but I don't think that we had a clear use case / spec for doing so. In 1.1.0 we performed a refactoring breaking the support for this. We'll work on a patch re-implementing support for this use case in v1.1.1.

[whoabuddy]

Thanks for the update! This is the first use case we've come up with, and more wanted to confirm what the expected behavior should be so we can plan the tests accordingly. We pinned 1.0.6 for now and will wait for the next release.

[whoabuddy]

@hugocaillard I ran some tests today using clarinet-cli 1.1.0 from the develop branch based on #680, and we're seeing an error now using the shorthand notation for a contract name, e.g.

error: expecting expression of type '(principal ST1PQHQKV0RJXZFY1DGX8MNSNYVE3VGZJSRTPGZGM.base-dao)', found 'principal'
--> contracts/extensions/ccd001-direct-execute.clar:57:7
      (is-eq tx-sender .base-dao)
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
error: expecting expression of type '(principal ST1PQHQKV0RJXZFY1DGX8MNSNYVE3VGZJSRTPGZGM.base-dao)', found 'principal'
--> contracts/extensions/ccd002-treasury.clar:35:7
      (is-eq tx-sender .base-dao)

[lgalabru]

hey @whoabuddy! the release/next branch (in PR here https://github.com/hirosystems/clarinet/pull/689) is fixing this issue.

[whoabuddy]

Testing out clarinet-cli 1.2.0 there and see a few errors regarding passing a smaller list than what's defined in the contractt:

error: expecting expression of type '(list 200 (tuple (enabled bool) (extension principal)))', found '(list 3 (tuple (enabled bool) (extension principal)))'
--> contracts/proposals/ccip012-bootstrap.clar:7:7
      (list
      ^
--
error: expecting expression of type '(list 100 (tuple (enabled bool) (token principal)))', found '(list 2 (tuple (enabled bool) (token principal)))'
--> contracts/proposals/ccip-test-treasury-003.clar:13:4
			(list
   ^
--
error: expecting expression of type '(list 100 (tuple (enabled bool) (token principal)))', found '(list 3 (tuple (enabled bool) (token principal)))'
--> contracts/proposals/ccip-test-treasury-002.clar:19:4
			(list
   ^
--
error: expecting expression of type '(list 100 (tuple (enabled bool) (token principal)))', found '(list 2 (tuple (enabled bool) (token principal)))'
--> contracts/proposals/ccip-test-treasury-005.clar:14:4
			(list
   ^

[lgalabru]

Indeed, that other issue was also in our visor. Should be addressed with latest.

[lgalabru]

@hugocaillard should we close this issue?

[hugocaillard]

We still have the issue that with the following configuration we'll analyze "counter.clar" twice.

[contracts.counter-1]
path = "contracts/counter.clar"

[contracts.counter-2]
path = "contracts/counter.clar"

This could be optimized, we can keep it open if we think it's important