-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support two levels of smart contract inheritance #1028
base: release/1.3.4.0
Are you sure you want to change the base?
Support two levels of smart contract inheritance #1028
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this change is going to cause mismatch between nodes @fassadlr .
You need to also update our validation tool as well @drmathias
What do you think about allowing implement multiple interfaces on contracts ? Currently it is limited to one interface or class.
{ | ||
return typeDefinition.IsClass && | ||
!typeDefinition.IsAbstract && | ||
typeDefinition.BaseType != null && | ||
typeDefinition.BaseType == typeof(SmartContract); | ||
InheritsFromType(typeDefinition, typeof(SmartContract)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already built-in method for it.
typeDefinition.IsSubclassOf(typeof(SmartContract))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know 👍
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drmathias Is this code is going to be used only by validator ? Can you check this ? Is there any other place it is used in fullnode ? We want to ensure that this code change will not cause consensus issues on network.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is only referenced to set the ContractAssembly.DeployedType
, which is used by the contract swagger doc generator and ReflectionVirtualMachine.Create
. I assume this would cause consensus issues, if a smart contract with 2 levels of inheritance was created on the network. In the example referenced above, updated versions of the node would execute the constructor of NonFungibleTicket
while older versions of the node would execute the constructor of NonFungibleToken
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this assumption is not correct. Tyler is able to deploy and run inherited types indirectly already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because he's creating a deployer contract, which directly inherits from SmartContract
. To clarify, I mean that it could cause consensus issues, if a smart contract with 2 levels of inheritance, which was decorated with the [Deploy]
attribute was created on the network.
src/Stratis.SmartContracts.CLR.Tests/Loader/ContractAssemblyTests.cs
Outdated
Show resolved
Hide resolved
The validation tool already treats 2+ levels of inheritance as valid.
I'm not sure this is the case. I seem to be able to compile a contract that implements 2 interfaces and then create it on the |
Can you provide minimal example that you tested and worked ? I remember Tyler was able to do this indirectly. It is like having a single contract class which creates(deploys) another smart contract that has 2 level inheritance. The class which has deploy attribute can't do this. |
See this https://github.com/drmathias/SmartContractMultipleInterfacesSample |
@YakupIpek are you happy with this PR? |
I guess this change is going to cause mismatch between nodes @fassadlr . |
The idea seems good in principle. I assume the SCT tool will need updating as well and is |
Takes into account indirect inheritance of
SmartContract
when determining the deploy type in a smart contract assembly. This allows contracts to be deployed with an inheritance structure like so:NonFungibleTicket
->NonFungibleToken
->SmartContract
.