[SC] Contract libraries (proposal) #506

rowandh · 2021-04-08T07:55:59Z

There's an architectural flaw in the original smart contracts design. Functionality such as hashing, serialization and now ecrecover are exposed via methods on the base SmartContract class.

This is not easily extensible as the SmartContracts class must be updated every time new functionality is added. Additionally it pollutes the SmartContracts class with unnecessary functionality.

This PR adds the beginning of a Smart Contract Library Stratis.SCL. The library is whitelisted for use in contracts. To add additional functionality, only the library module needs to be updated.

I've added EcRecover to demonstrate the proposed idea.

rowandh · 2021-04-08T08:08:02Z

@mrtpain would be great to hear your thoughts on this!

rowandh · 2021-04-08T08:20:39Z

If this is accepted then I will update #414 and stratisproject/Stratis.SmartContracts#14 and revert stratisproject/Stratis.SmartContracts#13

YakupIpek · 2021-04-08T21:47:02Z

It is possible to implement these features inside main library without modifying SmartContract class like you do with static classes. Also sometimes modification is unavoidable like we supported uint128/uint256 types and we needed to modify persistent state and other classes.

In some cases we may need to update both library if we add another library. IMO it would be better to go with single library dependency because i don't see clear benefits.

I am open to hear other ideas .

rowandh · 2021-04-08T23:56:43Z

Thanks @YakupIpek. Definitely some modification is unavoidable. The goal of this is mainly so we can add additional "library-style" functionality more easily in the future. Eg. if we need more support for manipulating byte arrays or converting an enum to a byte 😃

Are you just suggesting that this functionality be moved to Stratis.SmartContracts? The original goal of the Stratis.SmartContracts package was just to be a very basic contract interface that rarely needed updating. It contains almost no logic.

My preference is therefore to separate concerns and create a new namespace for this and add it to the whitelist.

mrtpain · 2021-04-09T00:10:28Z

I think this is a good idea, it'll help keep the SmartContract class clean and let developers choose which added functionality they want to bring into their contracts or keep out entirely.

YakupIpek · 2021-04-09T00:29:46Z

The goal of this is mainly so we can add additional "library-style" functionality more easily in the future.
How this can make it easier to add new functionality compared to add it to Stratis.SmartContracts ?

I only see separation concerns on this.

Adding another library bring more work. If i update Stratis.SCL library then i have to update Stratis.SmartContracts references again and create pr for it. Already at the moment we have to update Fullnode, SCT tool, Local-Chain project, visual studio template project, sometimes wallet UI..etc and this bring one more project to update process so for this reason i am favor of single library IMHO. We can add another folder for the libraries as namespace in Stratis.SmartContracts library.

I would like to know @quantumagi idea on this.

This is just my suggestion and it is still up to you @rowandh

rowandh · 2021-04-09T01:02:27Z

One of the goals with this is that we don't have to update Stratis.SmartContracts every time we want to expose new functionality to a contract.

For example the EcRecover functionality can be exposed through this library, meaning we don't need a SmartContract.EcRecover method.

quantumagi · 2021-04-09T02:41:02Z

src/Stratis.SmartContracts.CLR/Stratis.SmartContracts.CLR.csproj

@@ -17,6 +17,7 @@
  </ItemGroup>

  <ItemGroup>
+    <ProjectReference Include="..\Stratis.SCL\Stratis.SCL.csproj" />


This reference should be removed as Stratis.SCL should only ever be referenced by SCs (see #506 (comment)). Here it should be dynamically loaded, perhaps into a custom AssemblyLoadContext. Also, it would be very robust if SCs are always provided with the version of the library that they were originally validated with (if possible).

Stratis.SCL (Package) <- Stratis.SmartContracts.CLR (Package) <- Stratis.Bitcoin.Features.Interop (Package) <- Stratis.Bitcoin.Features.SmartContracts (Package) <- Stratis.Benchmark (Package) <- Stratis.Bitcoin.Networks (Package) <- Stratis.Features.Collateral (Package) <- Stratis.Features.FederatedPeg (Package) <- Stratis.SmartContracts.Networks (Package) <- Stratis.StraxD

Unused reference, I have removed it

The reference still exists implicitly. See #506 (comment).

quantumagi · 2021-04-09T02:57:09Z

The main benefit of this library is that, if done in a certain way, it could avoid scenarios such as: Package A uses B which is not a package, and Stratis.SCL is used by B, then we may have to bump the version of A if Stratis.SCL changes.

The guaranteed way to accomplish this is to only ever reference the Stratis.SCL library from smart contracts.

src/Stratis.SmartContracts.CLR.Validation.Tests/SmartContractDeterminismValidatorTests.cs

quantumagi · 2021-04-09T03:54:19Z

src/Stratis.SmartContracts.CLR.Validation/FormatPolicy.cs

@@ -23,7 +23,8 @@ public static class FormatPolicy
            Core,
            typeof(SmartContract).Assembly,
            typeof(Enumerable).Assembly,
-            typeof(IStandardToken).Assembly
+            typeof(IStandardToken).Assembly,
+            typeof(SCL.Base.Operations).Assembly


Should SCL.Crypto.ECRecover be added here? Ah wait it's the same assembly as Base right? It probably won't hurt adding it anyway if the list is made distinct as well (if required). Actually the SCL typeof references here may be problematic.

quantumagi · 2021-04-09T03:57:14Z

src/Stratis.SmartContracts.CLR/Compilation/ReferencedAssemblyResolver.cs

@@ -23,7 +23,8 @@ public static class ReferencedAssemblyResolver
                Core, 
                typeof(SmartContract).Assembly, 
                typeof(Enumerable).Assembly,
-                typeof(IStandardToken).Assembly
+                typeof(IStandardToken).Assembly,
+                typeof(SCL.Base.Operations).Assembly


See previous comment.

src/Stratis.SmartContracts.IntegrationTests/SmartContracts/EcRecoverContract.cs

quantumagi

LGTM. Just a few minor comments.

Reference still exists

quantumagi · 2021-04-09T04:26:29Z

quantumagi · 2021-04-09T04:42:07Z

src/Stratis.SmartContracts.CLR.Validation/Stratis.SmartContracts.CLR.Validation.csproj

@@ -13,4 +13,8 @@
    <PackageReference Include="Stratis.SmartContracts.Standards" Version="2.0.0" />
  </ItemGroup>

+  <ItemGroup>
+    <ProjectReference Include="..\Stratis.SCL\Stratis.SCL.csproj" />


Should not have this reference.

The validator needs to access types from it.

Yes, I noticed that. Could it access types by name (instead of typeof) or is there some other workaround? Otherwise we will have an avalanche of version updates each time Stratis.SCL is changed as shown here: #506 (comment). That would defeat the purpose of this library. I think that is what @YakupIpek's concern is. We need to handle this library in a special way to see any undeniable benefits,

quantumagi · 2021-04-09T04:46:28Z

Please ensure that all packages that have been changed or that have project reference to other packages that have updated versions have their versions bumped iff their versions are still the same as the latest package release. The new EnsureVersionsBumpedWhenChangingPublishedPackages test case (currently passing) should check this but it may still be worth double-checking.

rowandh · 2021-04-12T11:21:17Z

To avoid the various versioning issues noted by @quantumagi, I've moved everything into the Stratis.SmartContracts.CLR project. This also guarantees that SCL will always be loaded in the assembly load context.

I have also moved validation policy definitions out of Stratis.SmartContracts.CLR.Validation and into Stratis.SmartContracts.CLR which is where they should have been, and bumped both packages a full version as these are significant API changes.

Lastly I will note that this does not solve the 'library upgradeability problem', which is also present with #414. This just makes it possible to add new library functionality without lumping it on the base SmartContract class.

quantumagi

This PR shows 58 files changed. That does not seem right.

Can you resubmit your changes on top of the latest "infracontracts" branch or after first merging with it?

rowandh · 2021-04-12T12:30:38Z

Retargeted to infracontracts

rowandh requested review from quantumagi, fassadlr and YakupIpek April 8, 2021 08:06

rowandh force-pushed the contract-libraries branch from ee1a467 to 09c54d2 Compare April 8, 2021 13:18

quantumagi reviewed Apr 9, 2021

View reviewed changes

stratisproject deleted a comment from rowandh Apr 9, 2021

quantumagi reviewed Apr 9, 2021

View reviewed changes

src/Stratis.SmartContracts.CLR.Validation.Tests/SmartContractDeterminismValidatorTests.cs Show resolved Hide resolved

quantumagi reviewed Apr 9, 2021

View reviewed changes

src/Stratis.SmartContracts.IntegrationTests/SmartContracts/EcRecoverContract.cs Outdated Show resolved Hide resolved

quantumagi self-requested a review April 9, 2021 04:05

quantumagi previously approved these changes Apr 9, 2021

View reviewed changes

quantumagi self-requested a review April 9, 2021 04:25

quantumagi reviewed Apr 9, 2021

View reviewed changes

quantumagi self-requested a review April 12, 2021 08:22

quantumagi approved these changes Apr 12, 2021

View reviewed changes

rowandh changed the base branch from master to infracontracts April 12, 2021 11:15

quantumagi self-requested a review April 12, 2021 12:14

quantumagi requested changes Apr 12, 2021

View reviewed changes

rowandh force-pushed the contract-libraries branch from aee15c5 to c0e45cf Compare April 12, 2021 12:18

rowandh changed the base branch from infracontracts to master April 12, 2021 12:20

rowandh changed the base branch from master to infracontracts April 12, 2021 12:20

rowandh added 14 commits April 12, 2021 22:24

Add SCL to compiler

60d496b

Add SCL to validation

3b14874

Add tests

0684605

Add ECRecover and Keccak256

70b6d83

Add EcRecover integration tests

546abc4

Whitelist SCL.Crypto

ee1424b

Remove SignMessage

ce45fa8

Add error handling and adjust signature

215e249

Remove unused reference

b8666ca

Remove ECRecover implementation

ec35a8d

Move SCL into Stratis.SmartContracts.CLR

cbb68b2

Move validation policies to Stratis.SmartContracts.CLR

ffe289f

Bump version

b33623b

Bump validation version

b4393f9

rowandh force-pushed the contract-libraries branch from c0e45cf to b4393f9 Compare April 12, 2021 12:29

quantumagi approved these changes Apr 12, 2021

View reviewed changes

rowandh merged commit ff8ae49 into stratisproject:infracontracts Apr 13, 2021

quantumagi mentioned this pull request Jan 19, 2023

Add Stratis.SCL with ECRecover #1104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SC] Contract libraries (proposal) #506

[SC] Contract libraries (proposal) #506

rowandh commented Apr 8, 2021 •

edited

Loading

rowandh commented Apr 8, 2021

rowandh commented Apr 8, 2021

YakupIpek commented Apr 8, 2021

rowandh commented Apr 8, 2021

mrtpain commented Apr 9, 2021

YakupIpek commented Apr 9, 2021

rowandh commented Apr 9, 2021 •

edited

Loading

quantumagi Apr 9, 2021 •

edited

Loading

rowandh Apr 9, 2021

quantumagi Apr 9, 2021 •

edited

Loading

quantumagi commented Apr 9, 2021 •

edited

Loading

quantumagi Apr 9, 2021 •

edited

Loading

quantumagi Apr 9, 2021

quantumagi left a comment

quantumagi commented Apr 9, 2021 •

edited

Loading

quantumagi Apr 9, 2021

rowandh Apr 9, 2021

quantumagi Apr 9, 2021 •

edited

Loading

quantumagi commented Apr 9, 2021 •

edited

Loading

rowandh commented Apr 12, 2021 •

edited

Loading

quantumagi left a comment •

edited

Loading

rowandh commented Apr 12, 2021

[SC] Contract libraries (proposal) #506

[SC] Contract libraries (proposal) #506

Conversation

rowandh commented Apr 8, 2021 • edited Loading

rowandh commented Apr 8, 2021

rowandh commented Apr 8, 2021

YakupIpek commented Apr 8, 2021

rowandh commented Apr 8, 2021

mrtpain commented Apr 9, 2021

YakupIpek commented Apr 9, 2021

rowandh commented Apr 9, 2021 • edited Loading

quantumagi Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

rowandh Apr 9, 2021

Choose a reason for hiding this comment

quantumagi Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

quantumagi commented Apr 9, 2021 • edited Loading

quantumagi Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

quantumagi Apr 9, 2021

Choose a reason for hiding this comment

quantumagi left a comment

Choose a reason for hiding this comment

quantumagi commented Apr 9, 2021 • edited Loading

quantumagi Apr 9, 2021

Choose a reason for hiding this comment

rowandh Apr 9, 2021

Choose a reason for hiding this comment

quantumagi Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

quantumagi commented Apr 9, 2021 • edited Loading

rowandh commented Apr 12, 2021 • edited Loading

quantumagi left a comment • edited Loading

Choose a reason for hiding this comment

rowandh commented Apr 12, 2021

rowandh commented Apr 8, 2021 •

edited

Loading

rowandh commented Apr 9, 2021 •

edited

Loading

quantumagi Apr 9, 2021 •

edited

Loading

quantumagi Apr 9, 2021 •

edited

Loading

quantumagi commented Apr 9, 2021 •

edited

Loading

quantumagi Apr 9, 2021 •

edited

Loading

quantumagi commented Apr 9, 2021 •

edited

Loading

quantumagi Apr 9, 2021 •

edited

Loading

quantumagi commented Apr 9, 2021 •

edited

Loading

rowandh commented Apr 12, 2021 •

edited

Loading

quantumagi left a comment •

edited

Loading