Create TxIx that is backed by Word16 instead of an Int
#2637
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -17,7 +17,7 @@ transaction = | |||
| , auxiliary_data / null | |||
| ] | |||
|
|
|||
| transaction_index = uint | |||
| transaction_index = uint .size 2 | |||
lehins
force-pushed
the
lehins/add-txix
branch
from
66b09e6
to
981a04d
Compare
lehins
force-pushed
the
lehins/add-txix
branch
from
981a04d
to
da067ce
Compare
lehins
force-pushed
the
lehins/add-txix
branch
from
da067ce
to
d0130fd
Compare
nc6
approved these changes
Feb 2, 2022
| . C.newStakeCred Cast.aliceSHK (Ptr (SlotNo 10) 0 0) | ||
| . C.newStakeCred Cast.bobSHK (Ptr (SlotNo 10) 0 1) | ||
| . C.newStakeCred Cast.carlSHK (Ptr (SlotNo 10) 0 2) | ||
| . C.newStakeCred Cast.aliceSHK (Ptr (SlotNo 10) minBound (mkCertIxPartial 0)) |
There was a problem hiding this comment.
The inconsistent use of minBound vs mkCertIxPartial 0 niggles at me, but is really irrelevant :-D
There was a problem hiding this comment.
lol. 😄 I had the same thought when I was writing it, but decided to force myself to ignore it as irrelevant.
| deriving (Show, Eq, Ord, Generic, NFData, NoThunks) | ||
| deriving (ToCBOR, FromCBOR) via CBORGroup Ptr | ||
|
|
||
| -- | With this pattern synonym we assume that SlotNo increment rate isn't going | ||
| -- to speed up in the near future because with current rate we should be fine | ||
| -- for a few hundred years. |
There was a problem hiding this comment.
I might be inclined to expand upon this (how you're packing the slot and indices into a Word64, thus leaving only Word32 space for the slot)
There was a problem hiding this comment.
Very good suggestion. I expended on the compacting logic
|
Still LGTM! |
lehins
force-pushed
the
lehins/add-txix
branch
from
d0130fd
to
8bacafd
Compare
lehins
force-pushed
the
lehins/add-txix
branch
from
8bacafd
to
92c338c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Majority of the changes in this PR are to test suites, but the juice of it is in the
Cardano.Ledger.Credentialsmodule:TxIxfor type safety and memory optimizationCertIxfor type safety and memory optimizationPtrinto aWord64, since we can assume thatSlotNoisn't going to reach2^32 - 1value any time soon and other members ofPtrwere switched toWord16in the steps above.Left notes for future generations about the limits, hopefully they won't miss them.