Fix [iu](128|256) BSATN implementations in C# #1582
Merged
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.
Description of Changes
128- and, as of recently, 256-bit integers were implemented incorrectly in C#. They would write upper half of the number first, followed by the lower part, making the resulting encoding mixed-endian.
Unfortunately, this flew under the radar because SDK tests only verify round-trip: reducer arguments are passed in via BSATN encoding, and the table events are also retrieved in BSATN encoding.
As a result, if any encoding implementation is incorrect in an idempotent way - that is, you can only observe the bug from inside a module via stringification or arithmetic operations, but roundtrip restores the correct value - the SDK test doesn't see anything wrong with it, as what happened here.
I added one more test that, instead of round-tripping, sends values to the module and expects it to stringify them and insert strings into a table. This allows us to see the deserialized values as the module does, without going back via the original BSATN.
To implement this, I added proper stringification of large integers to C#, going via BigInteger conversion, which can be somewhat costly, but unlikely to matter in ToString().
I also extended sample integers in the primitive struct example to fill all bytes, as previously one half of int128/int256 would be unused and stay zero, which can also hide potential issues.
I want to extract common parts later and perhaps extend tests to do real property testing in C# instead of simply hardcoding values, but for now I'm satisfied this particular issue is fixed so wanted to submit as a separate PR.
API and ABI breaking changes
If this is an API or ABI breaking change, please apply the
corresponding GitHub label.
Expected complexity level and risk
How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.
This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.
If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.
Testing
Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!