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 Tokens contract #518

Closed
wants to merge 23 commits into from

Conversation

jmjatlanta
Copy link

NOTE 1: This builds on the jmj_ccinit branch. It will make for easier review if PR #444 was reviewed first.
NOTE 2: There have undoubtedly been a number of logic changes to the tokens contract as part of the Tokel project, Merging this into those changes will probably be difficult. If the ideas in this PR are desirable, it may be best to replay these commits after the Tokel changes are merged in (or redo this PR manually).

The functionality of the CC Tokens code was spread within several files. This PR moves them into a few, and wraps then into a class. In some cases, this eliminated the need to pass the CCcontract_info object as a parameter. In other cases, this required the call to static methods within the class.

NOTE 3: the function CCTokens::GetCCaddress is an example where converting this from a pseudo-override type function of (::GetCCaddress) to a template would avoid the runtime hit of indirection as well as increase type safety by removal of a few dynamic_casts, in exchange for increased compile time. ::GetCCaddress may be a candidate for something that should be within the CCcontract_info class, but that has not been explored.

/***
* Default ctor, sets everything to 0
*/
CCcontract_info() { memset(this, 0, sizeof(CCcontract_info)); }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the memset function is used to nullify fields of CCcontract_info class. Virtual table pointer (vptr) could be damaged, bcz memset doesn't know anything about vptr and sizeof(CCcontract_info) is virtual methods existence dependent. May be such default ctor with member initializer list will be more correct?

struct CCcontract_info
{

    uint8_t evalcode;
    uint8_t additionalTokensEvalcode2 = 0;
    char unspendableCCaddr[64];
    // < members skipped >
    CCcontract_info():   evalcode(0), additionalTokensEvalcode2(0),
                         unspendableCCaddr{0} /*, etc. */
                         {}
    virtual ~CCcontract_info() {}
    // < virtual methods skipped >
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i'd prefer to avoid using memset with c++ objects, as it may override members' constructor initialisations

@dimxy
Copy link
Collaborator

dimxy commented Nov 19, 2021

found one catch
there is a MakeCC1vout function in CCutils.cpp
and in tokens there was an additional function MakeTokensCC1vout (which does the same always add token evalcode). Now the MakeTokensCC1vout func is renamed also to MakeCC1vout.
in CCTokens::CreateToken() they are called both. But now the global MakeCC1vout (intended to be called) is obscured by CCTokens::MakeCC1vout, so now we need to call it with ::MakeCC1vout
That is, MakeTokensCC1vout was not planned to be an overload but an additional function

let's provide py tests to work, this will make life easier

@jmjatlanta
Copy link
Author

But now the global MakeCC1vout (intended to be called) is obscured by CCTokens::MakeCC1vout, so now we need to call it with ::MakeCC1vout That is, MakeTokensCC1vout was not planned to be an overload but an additional function

I thought I took care of that, but I'll research all of these issues. Thanks @DeckerSU and @dimxy for the reviews.

@jmjatlanta
Copy link
Author

jmjatlanta commented Nov 23, 2021

I was certainly calling the incorrect method for MakeCC1vout. I believe that has been fixed. I will also attempt to build a test to prove that it is fixed.

As for the vtable issue, I was certainly stepping on the pointer. Fixing this requires a sizable adjustment. As currently implemented, CCcontract_info needs to be in the common lib in order for the virtual method implementations to link correctly. That comes with a lot of unintended side effects that I want to avoid. I am taking a step back to see what other options are available.

Update: vtable issue fixed. Solution was to use pure virtual on 1 method that is always overridden, and not virtualize a method that never is overriden. Embarrassingly obvious.

@jmjatlanta jmjatlanta force-pushed the jmj_cctokens_refactor branch from 36564c4 to 2bcc883 Compare November 23, 2021 13:21
@jmjatlanta
Copy link
Author

As this affects a CC contract, it has been suggested (and I agree) that such changes not be done on the dev branch but some other branch (i.e. research).

For the moment, I will leave this open, but I suggest that review of this PR be a (very) low priority.

@dimxy
Copy link
Collaborator

dimxy commented Sep 16, 2022

This PR should be retargeted to the 'research' branch (where cc code is developed first)

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

5 participants