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

blockchain: move chain,skeleton to blockchain #3548

Closed
wants to merge 3 commits into from
Closed

Conversation

g11tech
Copy link
Contributor

@g11tech g11tech commented Jul 27, 2024

move skeleton,chain from client to blockchain class

first cut after a few iterations

  • doesn't yet merges db ops of skeleton to dbmanager, could be done in followup
  • need to ensure that skeleton to main db movement is minimzed (or even eliminated?)

aim of PR is to get and keep everything working

TODO:

  • readd chain updated event back
  • find the correct structure for status logging and its bookkeeping variables
  • fix the tests and all

@holgerd77
Copy link
Member

holgerd77 commented Aug 5, 2024

Hi Gajinder, had some first look, great that you started with this!

I honestly wonder a bit if this approach will work, to move all this stuff over that we have in client now. There is so much bloat and stuff we do not want to have in a generic blockchain class at the end, since all too client-specific.

So I wonder if it would be better to start the other way around: to try to expand Blockchain (somewhat from scratch) to allow for a less strict Skeleton version (not thinking about client at all), which might e.g. be the superclass of blockchain and just allows for a "non full" blockchain (likely by missing out some checks which are then done in the subclass) and then one could write some tests so that everything works and integrate this into the client instead of the current Skeleton. 🤔

However you do (there might be other ways), we should really make sure that we not totally bloat the Blockchain class. Blockchain should still remain a generic package - not tailored in any way (or: too much) for our client - and also serve as a generic package for whatever usages as a first-class use case.

@g11tech
Copy link
Contributor Author

g11tech commented Aug 8, 2024

Understood @holgerd77 will rethink the approach and rework

@g11tech
Copy link
Contributor Author

g11tech commented Aug 13, 2024

closing in favor of #3584 which aims to re-imagines this as per the feedback

@g11tech g11tech closed this Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants