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 All The Things - part1 #928

Merged
merged 3 commits into from
Aug 5, 2016
Merged

Refactor All The Things - part1 #928

merged 3 commits into from
Aug 5, 2016

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Aug 5, 2016

Alright... Time to clean up my diff backlog :)

I apologize in advance for a somewhat huge PR but things are way too interconnected here and also changes are mostly trivial. I tried to break it into 3 more or less separated commits though, so hopefully this will help to make review a bit easier. Also would like to mention that this is the first part of it and there is more to come in following PRs but I need this as a base (together with #911), so please don't put it at the end of your review queue :D

In general all of this is simply trying to:

  • move things out of .h (besides obvious one-liners)
  • clean up dependency lists a bit
  • simplify constructions to avoid "spaghetti code" as much as possible
  • name things properly - nSmth for int-s, fSmth or isSmth for bool-s etc.
  • follow code convention for braces and stuff (more or less)
  • unify log format for modules that were refactored
  • decouple from util.h and version.h
  • make some small additional fixes along the way, see below
  1. Refactor CActiveMasternode
  • move strMasterNodeAddr out of util.h to CActiveMasternode
  1. Refactor InstantSend
  1. Refactor Privatesend
  • more functions for CDarksendBroadcastTx: constructors, signing, serialization
  • use insecure_rand() instead of rand() in general but GetRand() for session id
  • new DEFAULT_...
  • move nPrivateSendRounds, nPrivateSendAmount, nLiquidityProvider, fEnablePrivateSend, fPrivateSendMultiSession, darkSendDenominations out of util.h

Note: vin part of some names was changed to txin (which is a proper name for an object of CTxIn because vin stands for vector of CTxIns) where applicable but not everywhere. That's intentional, I'll explain this in (one of?) the next part :)

@@ -1,4 +1,3 @@

// Copyright (c) 2009-2012 The Dash Core developers

Choose a reason for hiding this comment

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

nit: Copyright (c) 2014-2016 The Dash Core developers

@schinzelh
Copy link

utACK 9bc414c - love it ❤️

Shall we squash for merge or keep it as is?

@UdjinM6
Copy link
Author

UdjinM6 commented Aug 5, 2016

@schinzelh nit addressed. I'm fine with either way, I separated them only to make review easier :)

@UdjinM6
Copy link
Author

UdjinM6 commented Aug 5, 2016

Ooops, forgot to include CActiveMasternode decoupling from util.h (for strMasterNodeAddr). Now it should be fine, let's see what travis say :)
(updated first post)

@crowning-
Copy link

LOVE it ❤️

Much easier to read and understand now.

100% utACK

@schinzelh
Copy link

After merging #911 this needs a rebase now :)

UdjinM6 added 3 commits August 5, 2016 21:55
+ move strMasterNodeAddr to CActiveMasternode
+ new lock cs_instantsend to protect maps on CleanTransactionLocksList()
+ new DEFAULT_INSTANTSEND_DEPTH constant
+ rename MIN_INSTANTX_PROTO_VERSION to MIN_INSTANTSEND_PROTO_VERSION and bump it
+ decouple from util.h and version.h
+ more functions for CDarksendBroadcastTx: constructors, signing, serialization
+ move from rand() to insecure_rand() in general but to GetRand() for session id
+ fix defaults
@schinzelh
Copy link

utACK d24182c

@schinzelh schinzelh merged commit 5a8c0c9 into dashpay:v0.12.1.x Aug 5, 2016
@eduffield222
Copy link

lol, holy crap udjin 👍 I need a bit of time to review this, but I'll get to it soon :D

@UdjinM6 UdjinM6 deleted the refactor branch August 28, 2016 12:23
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