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

Separate KOMODO_STATES and CURRENCIES #484

Closed
wants to merge 4 commits into from

Conversation

jmjatlanta
Copy link

@jmjatlanta jmjatlanta commented Aug 23, 2021

Part of #479

The existing code looks up a pointer to komodo_state in an array, indexed by CURRENCIES.

This PR breaks that link, allowing the collection of komodo_state objects to

  • Lazily grow - The komodo_state objects are created as needed
  • Logarithmic lookups - the need to look up an index first is not necessary
  • hold the symbol - Interrogate the object for the symbol instead of maintaining one locally

This PR also

  • hide the CURRENCIES array behind a method. `komodo_currency(uint8_t) will return the currency symbol of a particular index.
  • Breaks the two things that happened in komodo_stateptr (getting the symbol and the pointer) into two methods. This allows methods that do not need the symbol and dest values to not create variables for them.
  • Override komodo_stateptr to give it the functionality of komodo_stateptrget. This provides a unified interface.

komodo_stateptr() now returns a pointer to komodo_state for the default chain.
komodo_stateptr(char *) now returns a pointer to komodo_state for the passed-in symbol.
komodo_nameset(char*, char*, const char*) can be used to set the symbol and dest as komodo_stateptr used to.

Note: It seems the pax_ routines use the currency array quite a bit. I am unsure of the functionality of this code. This area should be tested heavily to assure no functionality is broken.

@jmjatlanta jmjatlanta marked this pull request as ready for review September 13, 2021 17:03
@ca333 ca333 requested review from DeckerSU and dimxy January 11, 2022 10:35
@dimxy
Copy link
Collaborator

dimxy commented Jan 17, 2022

I think these changes are great, however as they touch the notary code I'd suggest someone tries them on a NN node (@DeckerSU)

@dimxy
Copy link
Collaborator

dimxy commented Jan 17, 2022

probably this PR will need to be rebased after jmj_npoints is merged (f.e. it still uses sp->NUM_NPOINTS replaced on NPOINTS.size() in the jmj_npoints)

Copy link
Collaborator

@dimxy dimxy left a comment

Choose a reason for hiding this comment

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

Changes look great and there are tests for them but I still suggest a NN operator would try to use them and check for correct work (absence of errors in logs and console)

@DeckerSU
Copy link

DeckerSU commented Jan 26, 2022

I agree with @dimxy , changes looks very good, but before detail review and logic check i should mention one thing, currencies array:

char CURRENCIES[][8] = { "USD", "EUR", "JPY", "GBP", "AUD", "CAD", "CHF", "NZD", // major currencies
    "CNY", "RUB", "MXN", "BRL", "INR", "HKD", "TRY", "ZAR", "PLN", "NOK", "SEK", "DKK", "CZK", "HUF", "ILS", "KRW", "MYR", "PHP", "RON", "SGD", "THB", "BGN", "IDR", "HRK",
    "KMD" };

Is related to PAX "stable-coins" experiment. So, all the code related to these PAX currencies (except KMD of course) is abandoned and will not ever used anymore. My personal feeling of this case - we should carefully remove all PAX related code from daemon, instead of trying to refactor corresponding sources. Probably @ca333 will have other vision, so what about this PR - we should discuss it first, to find a consensus.

My humble opinion and vision, as i said, we should carefully remove all PAX related code from daemon, for this PR it will mean to have only one komodo_state structure related to KMD. Also, in this case we will not need additional class to store all other currencies states. But, once again, this is only my vision and i would like to hear other opinions.

@jmjatlanta
Copy link
Author

jmjatlanta commented Jan 26, 2022

currencies array:
Is related to PAX "stable-coins" experiment.

If PAX code is indeed dead code, my vote would be to close or delay merging this PR, and scope-creep this into a "Remove PAX" PR.

Update: See PR #551

@TheComputerGenie
Copy link

Given that we're giving up on LABS, shouldn't that come out too?

@jmjatlanta
Copy link
Author

Closed in favor of #551

@jmjatlanta jmjatlanta closed this Jul 12, 2022
who-biz pushed a commit to who-biz/komodo that referenced this pull request Mar 14, 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.

4 participants