BenjaminMoe
commented
2 years ago Looks good to me. I'm guessing that the scheme of using 777777000500 doesn't look like it would cause collision issues. And it looks like it allocates enough digits that there shouldn't be overflow problems with transaction number or transaction number.
Edit: After reading the code more, it looks like TransactionNumber.maxTransactionIndexInBlock is being used, and checked against.
This was discovered in https://github.com/decentralized-identity/ion/issues/240
As a result, rewrote how transaction number is constructed to produce a smaller integer. Also took the opportunity to make the transaction number much more human-friendly also, the current transaction number format had been annoying me for a long time.
Specifically, the current transaction number is simply a large ugly looking number that doesn't make any sense to a human. It would be much easier to make sense of (thus debug), now that a transaction on index position 500 in block 777777 is 777777000500, as opposed to 3340526778581492. Notice the number is smaller AND easier to make sense of.
Also, due to this internal transaction number scheme change, both core and blockchain service will be required to be upgraded to the same build at the same time, a bit painful, but isn't worth more engineering time IMO to mitigate this inconvenience.