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

Replace ASSETCHAINS_SYMBOL with class #509

Closed

Conversation

jmjatlanta
Copy link

@jmjatlanta jmjatlanta commented Oct 8, 2021

The hope is to add more assetchain information into the class, with the ultimate goal of removing many of the related globals.

Other advantages (IMO):

  • clearer intent (ASSETCHAINS_SYMBOL[0] == 0 vs chain.isKMD())
  • fewer strcmp and strncmp calls (again, clearer intent)
  • some method signature changes (const vs non-const char*)

NOTE: This builds on the jmj_header_declarations branch, so either merge that one first (easier review) or close that one and merge this one (harder review).

@jmjatlanta jmjatlanta force-pushed the jmj_assetchains_symbol branch from 16b35b4 to 69dc615 Compare October 8, 2021 21:44
@jmjatlanta
Copy link
Author

While I believe this PR to be a step in the right direction, I recommend migrating to something within the NodeContext concept within more recent versions of Bitcoin Core. If others agree, please feel free to close this PR.

@tonymorony
Copy link

these changes implemented in combined PR: #559

@tonymorony tonymorony closed this Sep 5, 2022
Alrighttt pushed a commit to Alrighttt/komodo that referenced this pull request May 30, 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.

2 participants