Concerto CLI fails on some Accord Project models

[jeromesimeon]

Describe the bug
The CLI (e.g., concerto generate) fails on well formed and valid models from the Accord Project.
A clear and concise description of what the bug is.

To Reproduce

E.g., using examples from cicero-models:

bash-3.2$ cd ~/git/cicero-models/src/
bash-3.2$ concerto --version
0.80.4-20191003155034
bash-3.2$ concerto generate --ctoFiles signature/signature.cto 
Class ContractSigned is not declared as abstract. It must define an identifying field. File 'signature/signature.cto': line 9 column 1, to line 11 column 2.  IllegalModelException: Class ContractSigned is not declared as abstract. It must define an identifying field. File 'signature/signature.cto': line 9 column 1, to line 11 column 2. 
bash-3.2$ concerto generate --ctoFiles cicero/contract.cto 
Generated JSONSchema code.
bash-3.2$ concerto generate --ctoFiles cicero/runtime.cto 
Class Request is not declared as abstract. It must define an identifying field. File 'cicero/runtime.cto': line 13 column 1, to line 13 column 23.  IllegalModelException: Class Request is not declared as abstract. It must define an identifying field. File 'cicero/runtime.cto': line 13 column 1, to line 13 column 23. 
bash-3.2$ 

Additional context
This is most likely due to distinct systems models. See also #62

[vipulbhj]

I wanna work on this, can I try this if no one else is working on it @jeromesimeon

[dselman]

The issue is that the ModelManager is created without a system model:
https://github.com/accordproject/concerto/blob/master/packages/concerto-cli/lib/commands.js#L64

In AP we use this system model:
https://github.com/accordproject/ergo/blob/master/packages/ergo-compiler/lib/apmodelmanager.js#L21

A quick fix would be to use the same system model for concerto generate but we really should remove system models altogether to ensure that models are completely self-describing and reusable.

[vipulbhj]

If that is the case, I should patch this first with the above suggested approach and then once the bug is fixed we can decide how to remove system model completely.
what do you guys think ?

[jeromesimeon]

@vipulbhj Sounds like a pretty good plan.

For the first step, one question will be which system model since Accord Project has one and at least Hyperledger Composer has another, and other users may have used that capability and built their own. One possibility would be to have an option --ctoSystem which lets the user specify a system model. If we have a default system model, which should it be? I would vote for the Accord Project system model which is here: https://github.com/accordproject/models/blob/master/src/cicero/base.cto

The second step needs definitely a more complete proposal since this will affect every other project, and users. We need to decide what each kind of base type mean (Asset, Concept, Event, etc). I'll be working on this, so I'm happy to join forces!

[jeromesimeon]

For reference, the way to add the system model to the model manager is illustrated here (using the Accord Project system model): https://github.com/accordproject/ergo/blob/9e216d86c87c6b67f640e7982f0e639a792e6207/packages/ergo-compiler/lib/apmodelmanager.js#L44

[vipulbhj]

Thanks for the reference material. For the first step this does sound like a good plan. I be trying to dig more in the command code today and I will try to add the option as discussed. I am relatively new here so I am gone go with your suggestion for default, if that's okay

[jeromesimeon]

That's great @vipulbhj ! If you start a PR with some of the basic idea in place for that new option, I can review while the discussion about default model takes place.

Since cicero-cli is brand new, we do not have to worry whether this breaks anyone's code.

[jeromesimeon]

Just a caveat: this will probably a short lived fix, since we'll likely remove the notion of system model soon after!

[vipulbhj]

I will try to submit a PR soon

[vipulbhj]

Hey @jeromesimeon, made some first rough draft changes. Mind taking a look ??

[jeromesimeon]

Fixed in #113 . Thanks for your help @vipulbhj !