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

Refactor CryptoConditions to use more OO features #444

Closed
wants to merge 7 commits into from

Conversation

jmjatlanta
Copy link

@jmjatlanta jmjatlanta commented May 21, 2021

Fixes #441
Existing contracts were very "C"-like. Methods take long lists of parameters, methods scattered in different files, macros to generate functions, etc.

This PR takes a step in the OO direction. Contracts are now structs that derive from a base CCcontract_info struct. The former "macroed" functions are now simply a part of the base class, which can be overridden if need be.

A new contract will be a struct that derives from CCcontract_info and overrides the validate function. Any methods needed by the contract can be a class method, which aides in naming without collisions.

Common functionality that all (or at least most) contracts need can be moved into the CCcontract_info base class (i.e. GetUnspendable).

The CCinit method has now been simplified to return a shared pointer to the desired contract type. In addition, most methods have been modified to use the desired contract class instead of the (more generic) base class.

These changes keep a large portion of contract logic within the bounds of the contract itself. The hope is that eventually anything contract-specific will be in the corresponding .cpp file as a private method, and can rely on this instead of passing pointers around.

** Current Status ** This PR should contain no logic changes. The system should work exactly as before, with the exceptions of:

  • Contracts related to EVAL_FIRSTUSER are now on the heap instead of the stack (see eval.cpp:CCinfos[])
  • Some logic related to heir is also using CCinit, and hance using heap allocation.
  • CCinit can return nullptr in some cases (as could the previous version). This is often not being checked. In the past this was less of a problem, due to the stack allocation, although would probably lead to questionable logic issues. Calls to CCinit (which are now minimal) should be reviewed and protections added.

@jmjatlanta jmjatlanta marked this pull request as ready for review June 10, 2021 13:04
@ca333 ca333 requested review from dimxy and DeckerSU July 25, 2021 01:49
@dimxy
Copy link
Collaborator

dimxy commented Aug 6, 2021

I generally support refactoring CCcontract_info structure to make it more OOP-like.
I'd like to make a note that it actually is used for 2 cases:

  • provide needed data for the validation code
  • it is also used in FinalizeCCtx function to set signing info. For this option it contains several ad-hoc variables like coins1of2addr, tokens1of2addr, unspendableaddr2, unspendableaddr3 etc. These objects were added case by case and still don't cover all signing cases. Maybe we could replace all of them with a unified case. And there was an attempt to create such unified signing info feature:
    void CCAddVintxCond(struct CCcontract_info *cp, const CCwrapper &condWrapped, const uint8_t *priv)
    . It allows to set an array of probe cryptoconditions (in the CContract_info struct) that are checked against spent utxos and point out a privkey to sign if matched. I actively use this in the latest cc modules. If we are okay with it I suggest to add this variable in the renewed CContract_info too (or do something else)

@jmjatlanta
Copy link
Author

  . It allows to set an array of probe cryptoconditions (in the CContract_info struct) that are checked against spent utxos and point out a privkey to sign if matched. I actively use this in the latest cc modules. If we are okay with it I suggest to add this variable in the renewed CContract_info too (or do something else)

Interesting. I can not say I understand the full process, but at first glance it sounds good. Getting rid of variables that are rarely used by including their storage and logic only when they are used is a good thing IMO. I will try to learn more about the functionality you are adding.

@tonymorony tonymorony removed their request for review July 1, 2022 10:22
@dimxy
Copy link
Collaborator

dimxy commented Jul 17, 2022

I suggest we first make this improvement into a research-like branch to concentrate all cc changes there

who-biz pushed a commit to who-biz/komodo that referenced this pull request Aug 30, 2022
@tonymorony tonymorony closed this Oct 17, 2022
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