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

Remove support for system models #62

Closed
dselman opened this issue Sep 16, 2019 · 14 comments
Closed

Remove support for system models #62

dselman opened this issue Sep 16, 2019 · 14 comments

Comments

@dselman
Copy link
Contributor

dselman commented Sep 16, 2019

Support for system models has complicated the codebase and makes it hard to reuse models, as they now have an implicit dependency on a system model.

On balance I think we should make a breaking change and remove the support for system models, forcing people to explicitly declare dependencies on their own models using an import with a from.

@jeromesimeon
Copy link
Member

jeromesimeon commented Sep 16, 2019

A few comments:

  1. We need a definition for what the structure of a transaction event participant etc is. Those need to be built in in some sense
  2. One of the motivations for the current structure is to be able to switch those systems model (I think two main instances are for hyperledger composer and for Cicero). There are a set of defaults for hyperledger. I believe the code could be simplified if this requirement is no longer needed (not sure if it's acceptable to remove the hyperledger system model).
  3. If we revise the top of the type hierarchy, it might be nice to decide on a super type for all of classes (e.g., concept could be the super class for everything or it could be object or class)

@jeromesimeon
Copy link
Member

Why having a top type would be useful: accordproject/ergo#420

@dselman
Copy link
Contributor Author

dselman commented Sep 19, 2019

  1. Agreed
  2. Yes, I think this is acceptable. We can preserve the hyperledger system model as a "standard" model on models.accordproject.org so that people can import it into their models and refactor if required.
  3. Agreed. concept seems like a natural candidate, but that may have some API implications.

@jeromesimeon
Copy link
Member

jeromesimeon commented Oct 4, 2019

Small fact finding mission. I can't seem to find a clear dependency from Concerto to HL Composer:

bash-3.2$ grep -r concerto composer
Binary file composer/packages/composer-admin/test/data/businessnetwork.zip matches
composer/packages/composer-common/test/data/zip/test-npm-archive/node_modules/animaltracking-model/.npmignore:!chaincode/src/concerto
composer/packages/composer-common/test/data/zip/test-npm-archive/node_modules/animaltracking-model/models/mozart.cto:namespace com.ibm.concerto.mozart
Binary file composer/packages/composer-common/test/data/zip/test-archive.zip matches
Binary file composer/.git/objects/pack/pack-19a8cd93ba0365a189b8f632a9e5f3440428f9ae.pack matches
Binary file composer/.git/index matches

With that project being deprecated, can we assume supporting the hyper ledger composer system model is not a requirement?

@dselman
Copy link
Contributor Author

dselman commented Oct 4, 2019

Small fact finding mission. I can't seem to find a clear dependency from Concerto to HL Composer:

bash-3.2$ grep -r concerto composer
Binary file composer/packages/composer-admin/test/data/businessnetwork.zip matches
composer/packages/composer-common/test/data/zip/test-npm-archive/node_modules/animaltracking-model/.npmignore:!chaincode/src/concerto
composer/packages/composer-common/test/data/zip/test-npm-archive/node_modules/animaltracking-model/models/mozart.cto:namespace com.ibm.concerto.mozart
Binary file composer/packages/composer-common/test/data/zip/test-archive.zip matches
Binary file composer/.git/objects/pack/pack-19a8cd93ba0365a189b8f632a9e5f3440428f9ae.pack matches
Binary file composer/.git/index matches

With that project being deprecated, can we assume supporting the hyper ledger composer system model is not a requirement?

Yes. Composer still has an old/private copy of Concerto packaged with it - so those people will be fine.

@jeromesimeon
Copy link
Member

jeromesimeon commented Oct 4, 2019

Fact Finding Item Nb2. The current Accord Project system model is as follows:

namespace org.accordproject.base

/**
 * System model for Accord Project models. If you use the CiceroModelManager 
 * from the @accordproject/cicero-core npm module you do not need to import this file.
 */
abstract asset Asset {  }

abstract participant Participant {   }

abstract transaction Transaction identified by transactionId {
  o String transactionId
}

abstract event Event identified by eventId {
  o String eventId
}

Defined in: https://github.com/accordproject/models/blob/master/src/cicero/base.cto

@jeromesimeon
Copy link
Member

Small fact finding mission. I can't seem to find a clear dependency from Concerto to HL Composer:

bash-3.2$ grep -r concerto composer
Binary file composer/packages/composer-admin/test/data/businessnetwork.zip matches
composer/packages/composer-common/test/data/zip/test-npm-archive/node_modules/animaltracking-model/.npmignore:!chaincode/src/concerto
composer/packages/composer-common/test/data/zip/test-npm-archive/node_modules/animaltracking-model/models/mozart.cto:namespace com.ibm.concerto.mozart
Binary file composer/packages/composer-common/test/data/zip/test-archive.zip matches
Binary file composer/.git/objects/pack/pack-19a8cd93ba0365a189b8f632a9e5f3440428f9ae.pack matches
Binary file composer/.git/index matches

With that project being deprecated, can we assume supporting the hyper ledger composer system model is not a requirement?

Yes. Composer still has an old/private copy of Concerto packaged with it - so those people will be fine.

Ah, I see. Thanks for clarifying!

@jeromesimeon
Copy link
Member

jeromesimeon commented Oct 4, 2019

The fundamental role of the systems model is to define the type names Asset Participant Transaction and Event and the build-in attributes/fields in them (e.g., in the Accord Project system model Asset has no built-in field and Transaction has a single builtin transactionId field).

With that in mind I could see a couple of options:

  1. Fold the current Accord Project system model and make it built-in, so all user models assume the same definitions. A couple of decisions to make:
  • What namespace should it be in. Are we happy with org.accordproject.base.
  • Should we have Concept be a super type for the others, which would provide us with a well formed hierarchy with a root Concept type. This would be the equivalent of having extends Concept in those definitions.
  1. Remove the system model entirely and define those names the same way we define Concept:
    This might make more sense if the implied semantic is as if we had the following systems model:
namespace org.accordproject.base

abstract asset Asset {  }
abstract participant Participant {   }
abstract transaction Transaction {   }
abstract event Event {   }
  • Still similar decisions on namespace choice and whether to add an implicit extends Concept for all of those has to be made.

A drawback of 2. is that identifiers for some of those has to become part of the user models.
A benefit of 2. is that if anyone has used a system model with different assumptions from the AP ones (e.g., in which transactions do not have built-in identifier), those can more easily be ported.

@dselman @mttrbrts @sstone1 I would love your feedback on this.

@jeromesimeon
Copy link
Member

  • Agreed
  • Yes, I think this is acceptable. We can preserve the hyperledger system model as a "standard" model on models.accordproject.org so that people can import it into their models and refactor if required.
  • Agreed. concept seems like a natural candidate, but that may have some API implications.

One more question on this. Where can I find a definition of the hyperledger system model?

@jeromesimeon
Copy link
Member

jeromesimeon commented Oct 4, 2019

The fundamental role of the systems model is to define the type names Asset Participant Transaction and Event and the build-in attributes/fields in them (e.g., in the Accord Project system model Asset has no built-in field and Transaction has a single builtin transactionId field).

With that in mind I could see a couple of options:

  1. Fold the current Accord Project system model and make it built-in, so all user models assume the same definitions. A couple of decisions to make:
  • What namespace should it be in. Are we happy with org.accordproject.base.
  • Should we have Concept be a super type for the others, which would provide us with a well formed hierarchy with a root Concept type. This would be the equivalent of having extends Concept in those definitions.
  1. Remove the system model entirely and define those names the same way we define Concept:
    This might make more sense if the implied semantic is as if we had the following systems model:
namespace org.accordproject.base

abstract asset Asset {  }
abstract participant Participant {   }
abstract transaction Transaction {   }
abstract event Event {   }
  • Still similar decisions on namespace choice and whether to add an implicit extends Concept for all of those has to be made.

A drawback of 2. is that identifiers for some of those has to become part of the user models.
A benefit of 2. is that if anyone has used a system model with different assumptions from the AP ones (e.g., in which transactions do not have built-in identifier), those can more easily be ported.

@dselman @mttrbrts @sstone1 I would love your feedback on this.

[For what it's worth, I think that, currently, my preferred options would be 2. + extends Concept essentially equivalent to a built-in system model as follows:

namespace org.accordproject.base

abstract asset Asset extends Concept {  }
abstract participant Participant extends Concept {   }
abstract transaction Transaction extends Concept {   }
abstract event Event extends Concept {   }

]

@jeromesimeon
Copy link
Member

Just to make sure we are clear option 2. will break every Accord Project templates! There is certainly a very big benefit in 1. for backward compatibility in other Accord Project projects.

@dselman
Copy link
Contributor Author

dselman commented Oct 6, 2019

Thanks for the useful analysis @jeromesimeon. I suggest we take this up at the next WG meeting.

@jeromesimeon
Copy link
Member

Update on this. The release-1.0 branch includes full removal of application setting systems model. This uses a base system model internally which is as follows:

namespace system
abstract asset Asset {  }
abstract transaction Transaction { }
abstract event Event { }
abstract participant Participant {  }

Some remaining open issues are listed in the corresponding PR: https://github.com/accordproject/concerto/tree/release-1.0

Those relate to the question of whether we should have a root class for the type hierarchy (#180) and revisions we should discuss for identifiers and relationships (#181)

@jeromesimeon
Copy link
Member

@dselman Can we consider this closed based on the latest review and support in release-1.0. Remaining related questions (notably on relationships) have their own issues now.

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

No branches or pull requests

3 participants