Skip to content
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

Open
wants to merge 2 commits into
base: release/1.3.4.0
Choose a base branch
from

Conversation

drmathias
Copy link
Contributor

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.

@fassadlr fassadlr requested a review from YakupIpek July 27, 2022 09:12
Copy link
Member

@YakupIpek YakupIpek left a 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));
Copy link
Member

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))

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@drmathias
Copy link
Contributor Author

You need to also update our validation tool as well @drmathias

The validation tool already treats 2+ levels of inheritance as valid.

What do you think about allowing implement multiple interfaces on contracts ? Currently it is limited to one interface or class.

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 devmode version of the node.

@YakupIpek
Copy link
Member

YakupIpek commented Jul 28, 2022

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.

@drmathias
Copy link
Contributor Author

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

@fassadlr fassadlr changed the base branch from release/1.3.2.1 to release/1.3.4.0 August 11, 2022 10:57
@fassadlr
Copy link
Contributor

@YakupIpek are you happy with this PR?

@YakupIpek
Copy link
Member

@YakupIpek are you happy with this PR?

I guess this change is going to cause mismatch between nodes @fassadlr .

@quantumagi quantumagi self-requested a review September 22, 2023 03:07
@quantumagi
Copy link
Contributor

quantumagi commented Sep 22, 2023

The idea seems good in principle.

I assume the SCT tool will need updating as well and is Stratis.SmartContracts.CLR.Validation affected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants