perf: improve decoding in CallTransaction

[Vovchyk]

Description

Motivation and Context

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• Tests for the changes have been added (for bug fixes / features)
• Requires Activation Code (Hard Fork)

• Other information:

[fmacleal]

Good job. I have added some comments, mostly with improvements suggestions to improve the readability of some tests. They doesn't prevent the approval, since it wouldn't change the current logic.

[fmacleal]

suggestion:

Identify the scenarios you are testing it's a bit tricky, because we don't have clues with variable names and so on. I think we could get advantage of @ParameterizedTest and a String to tell us the scenario you are testing. Here is a snippet code in how you could change this test. I think that we could get benefit of it or use similar ideas for the other tests below.

Suggested change

	@Test
	void testBytesLength() {
	assertEquals(0, Bytes.of(new byte[]{}).slice(0, 0).length());
	assertEquals(0, Bytes.of(new byte[]{1}).slice(0, 0).length());
	assertEquals(1, Bytes.of(new byte[]{1}).slice(0, 1).length());
	assertEquals(0, Bytes.of(new byte[]{1,2,3}).slice(1, 1).length());
	assertEquals(1, Bytes.of(new byte[]{1,2,3}).slice(1, 2).length());
	assertEquals(2, Bytes.of(new byte[]{1,2,3}).slice(0, 2).length());
	assertEquals(3, Bytes.of(new byte[]{1,2,3}).slice(0, 3).length());
	}
	@ParameterizedTest
	@MethodSource("provideInputForBytesLengthTest")
	void testBytesLength(String testDescription, int expectedLength, byte[] inputArray, int from, int to) {
	assertEquals(expectedLength, Bytes.of(inputArray).slice(from, to).length());
	}
	static Stream<Arguments> provideInputForBytesLengthTest(){
	return Stream.of(
	Arguments.of("Length 0 for empty array", 0, new byte[]{}, 0,0),
	Arguments.of("Length 0 for slice with same from and to", 0, new byte[]{1}, 0,0),
	Arguments.of("Length 1 for slice with distance between from and to", 1, new byte[]{1,2,3}, 0,1),
	Arguments.of("Length 0 for empty array", 0, new byte[]{1,2,3}, 1,1),
	Arguments.of("Length 1 for slice with distance between from and to", 1, new byte[]{1,2,3}, 1, 2),
	Arguments.of("Length 2 for slice with distance between from and to", 2, new byte[]{1,2,3}, 0, 2),
	Arguments.of("Length 3 for slice with distance between from and to", 3, new byte[]{1,2,3}, 0, 3)
	);
	}

[Vovchyk]

thanks for the suggestion. I prefer the original version more. It seems to be kinda minimalistic and more readable

[fmacleal]

suggestion:

Break this test in two, one to test the Exceptions and other to validate the proper functionality of the method. You could also get use of the parameterized test here. HAving a single assertion with different inputs.

[fmacleal]

question:

Why the size is doubled? 😅
Because it become an object and it occupies more space? 🤔

[Vovchyk]

here we're limiting printing of byte sequences to 32 bytes. In stringular / hex form it'd be 64 chars

[fmacleal]

suggestion:

Although all variables are used here, I would suggest create local variables to hold the value of the arrays. So we can have a better understanding just by reading the code withoug have to interpret it. For example:

...
        byte[]  anotherDestArray= new byte[5];
        assertArrayEquals(anotherDestArray, dest); // yet unmodified

        fun.apply(origin, 0, dest, 0, 5);
        byte[] expectedDestAfterCopy = new byte[]{1,2,3,4,5};
        assertArrayEquals(expectedDestAfterCopy, dest);

        anotherDestArray = new byte[5];
        fun.apply(origin, 1, anotherDestArray, 1, 3);
        byte[] newExpectedArray = new byte[]{0,2,3,4,0};
        assertArrayEquals(newExpectedArray, anotherDestArray);

[fmacleal]

LGTM

[fmacleal]

pipeline:run

[nagarev]

LGTM

[sonarcloud]

Quality Gate passed

Issues
56 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.8% Coverage on New Code
1.5% Duplication on New Code

See analysis details on SonarCloud

[fmacleal]

LGTM

[fmacleal]

pipeline:run