Pull request #90 review

[sigmike]

There are multiple places in #90 where the comment // ppcoin: has been changed to // peercoin:. I'd rather keep them as they are in the peercoin source code. It will make future merges easier. We will have to merge peercoin codes later, so I'd rather only change what is necessary to avoid conflicts.

There are also changes from // ppcoin: to // peershares:. I think we should not do that. We should be able to identify what was changed from bitcoin to peercoin, and what was changed from peerocin to peershares. Again this will make future merges from bitcoin or peercoin easier.

And I think we should follow Sunny habits and add a // peershares: each time we change the code specifically for peershares. It's very convinient to know what belongs to bitcoin, peercoin or peershares when you read the source code.

The copyright has also been changed from The PPCoin developers to The Peercoin developers. It's probably a minor issue, but I don't think we are allowed do that.

Also at this line: https://github.com/Peershares/Peershares/pull/90/files#diff-c865a8939105e6350a50af02766291b7R299
The 7 should be changed 11 (at both places).
And on this line the first 7 must be changed too: https://github.com/Peershares/Peershares/pull/90/files#diff-8c9d79ba40bf702f01685008550ac100R144
And here too: https://github.com/Peershares/Peershares/pull/90/files#diff-8c9d79ba40bf702f01685008550ac100R272

Also on these lines: https://github.com/Peershares/Peershares/pull/90/files#diff-e8db9b851adc2422aadfffca88f14c91L734
I think this will only make things harder when we'll merge a new peercoin or bitcoin.

Here the example is wrong (the address is a bitcoin address): https://github.com/Peershares/Peershares/pull/90/files#diff-b20e37cb944b4e37fb38c3b2c402bc7bL1186

Here the filename should not change: https://github.com/Peershares/Peershares/pull/90/files#diff-b20e37cb944b4e37fb38c3b2c402bc7bL1629

And the same applies to other translation files.

Why do we remove this method?
https://github.com/Peershares/Peershares/pull/90/files#diff-1fc2d3d7edc00ab8ea29eb1ca30cdcbbL817

The other changes look good.

[brossi]

I'll get these updates processed this evening.

Regarding the 'ppcoin:' > 'peercoin:' changes, it's my intent to submit a pull-request to the Peercoin repo that will make the same changes. The protocol is no longer called "PPCoin", so I'm hoping that my changes will be accepted. In the case that they are, we won't have a plethora of merge conflicts when we automatically pull new code.

The method in serialize.h was removed because it was generating an error in OS X while building the QT client. It did not appear to make a change in the Linux build of the QT client, but I still need to finish off testing of the Gitian build process to make sure. If it does conflict, I'll need to pick your brain about add an if statement to handle the build environment.

For my own personal knowledge, can you give me a little background about the changes in checkpoints? What does the switch from 7 to 11 do? Is this something that people will change when they deploy their own Peershares networks, or is this a protocol update that is generic?

[sigmike]

What does the switch from 7 to 11 do?

These lines detect whether the string starts with ppcoin:. So they first check whether the line is at least 7 characters long (the size of ppcoin:) and then compare only the first 7 characters (the strncasecmp call).
Now that the string you want to find is longer you must also update the sizes.

Is this something that people will change when they deploy their own Peershares networks, or is this a protocol update that is generic?

Only if they change the length of the string like you did.

[brossi]

I thought about it during lunch and will revert the "// ppcoin: " comment changes to match the existing branch of Peercoin. Once I have time to submit a pull-request to that repo (isolated for those changes only), if it's picked up, we can pull those into the Peershares fork.

In the future I'll work with less "broad" strokes when it comes to areas of the code that are from the fork.