Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

fix: Add tests for json-rpc-data and address to describe expected behaviour #2716

Merged
merged 9 commits into from
Mar 31, 2022

Conversation

jeffsmale90
Copy link
Contributor

@jeffsmale90 jeffsmale90 commented Mar 28, 2022

fixes #1594

@jeffsmale90
Copy link
Contributor Author

I spent some time looking into this today, and have a reasonable grasp on what's going on.

I just wanted to check whether my expectations around the behaviour are correct, so this PR only contains a couple of tests describing that behaviour. Don't expect anything to work, and don't take this as a full and complete set of tests for this behaviour!

Thanks.

@jeffsmale90
Copy link
Contributor Author

If you have a look at the build, pretty much all of these tests currently fail, except for the first set where no byteLength parameter is passed, as in the current implementation, the byteLength parameter has no impact on the toString() result.

Copy link
Contributor

@MicaiahReid MicaiahReid left a comment

Choose a reason for hiding this comment

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

This looks good! If you implemented these changes I think it will give us a lot more confidence in our Data types. I think the biggest concern is all of the possible side affects this fix could cause. Let's hope our tests are robust enough to catch them!

One more thing to note that I realize as I was digging into the code for the Data constructor. It only allows a value of type string | Buffer, but we only validate against bigint. It might be worth adding extra validation? ¯_(ツ)_/¯

@jeffsmale90
Copy link
Contributor Author

There's plenty of opportunity to improve this - ie. I tested the performance impact of removing the weakmaps, and it's quite significant, but I wanted to fix the issue without changing too much, with the intention of addressing remaining problems when we rework this.

@jeffsmale90 jeffsmale90 marked this pull request as ready for review March 30, 2022 08:45
@jeffsmale90 jeffsmale90 changed the title fix: Add tests for json-rpc-data and address to describe expected behaviour #1594 fix: Add tests for json-rpc-data and address to describe expected behaviour fixes #1594 Mar 30, 2022
@jeffsmale90 jeffsmale90 changed the title fix: Add tests for json-rpc-data and address to describe expected behaviour fixes #1594 fix: Add tests for json-rpc-data and address to describe expected behaviour Mar 30, 2022
Copy link
Contributor

@MicaiahReid MicaiahReid left a comment

Choose a reason for hiding this comment

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

Looks good! Tiny nit-picks in the tests.

src/packages/utils/tests/json-rpc-data.test.ts Outdated Show resolved Hide resolved
src/packages/utils/tests/json-rpc-data.test.ts Outdated Show resolved Hide resolved
src/packages/utils/tests/json-rpc-data.test.ts Outdated Show resolved Hide resolved
src/packages/utils/tests/json-rpc-data.test.ts Outdated Show resolved Hide resolved
src/packages/utils/tests/json-rpc-data.test.ts Outdated Show resolved Hide resolved
@jeffsmale90
Copy link
Contributor Author

Lucky you suggested the change to use getExpectedString - because it didn't truncate properly!

The reason I had a couple of special cases explicitly defined in the tests was to explain the behaviour around truncating / padding. Using getExpectedString instead and adding the 32byte value to the validValues array, I found that the special cases were duplicates, so I removed them and added a comment to the getExpectedString function to explain.

@MicaiahReid
Copy link
Contributor

@jeffsmale90 I added one typo fix to that new comment, but I've approved.

@jeffsmale90 jeffsmale90 merged commit f2b5b8b into develop Mar 31, 2022
@jeffsmale90 jeffsmale90 deleted the fix/address_padding branch March 31, 2022 21:19
davidmurdoch pushed a commit that referenced this pull request Apr 4, 2022
MicaiahReid pushed a commit to domob1812/ganache that referenced this pull request Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix toString Padding for Address types
2 participants