neo icon indicating copy to clipboard operation
neo copied to clipboard

Improve code coverage for UT and fixed bug

Open chenzhitong opened this issue 1 year ago • 8 comments

chenzhitong avatar Jun 24 '24 03:06 chenzhitong

Thanks @cschuchardt88 , I'll keep adding UT

chenzhitong avatar Jun 24 '24 06:06 chenzhitong

@chenzhitong If you could please update the description of the 1st comment of what this PR covers. So when we merge, we will know.

cschuchardt88 avatar Jun 27 '24 07:06 cschuchardt88

I need some time to ensure that your update will not change existing behavior. for instance, if the original code should throw exception, it should not return null in your code.

Jim8y avatar Jun 28 '24 16:06 Jim8y

Base58CheckDecode's check may cause hard fork, we'd better have an empty string tested here on both older version and newer version

do-shwhy avatar Jul 02 '24 10:07 do-shwhy

Base58CheckDecode's check may cause hard fork, we'd better have an empty string tested here on both older version (line 39) and newer version

In both older and newer versions, Base58CheckDecode throws an exception when an empty string is entered.

image

In the old version, if(input is null) was always false, because the parameter type is string, not string?

chenzhitong avatar Jul 03 '24 02:07 chenzhitong

In the old version, if(input is null) was always false, because the parameter type is string, not string?

I thought that the empty string "" may cause differ results in the two versions. In previous version it will just return empty byte array new byte[]{} while it will throw exception in this new version?

Seems that the empty string will throw FormatException anyway. Sorry. Ignore my comment above. Let's go on.

One more concern is that I'm not sure that if the FormatException and ArgumentNullException could be detected by VM scripts. If there is a way for detecting such exceptions, maybe there still exists hardfork risks. Could you please take a look? @Jim8y

do-shwhy avatar Jul 03 '24 04:07 do-shwhy

Yes it will be detected by the ApplicationEngine.

cschuchardt88 avatar Jul 03 '24 05:07 cschuchardt88

Just in case, the exception type will not be modified here

chenzhitong avatar Jul 03 '24 05:07 chenzhitong

Not ready until changes are made.

cschuchardt88 avatar Jul 04 '24 16:07 cschuchardt88

@Jim8y do you want to check the last changes?

shargon avatar Jul 05 '24 07:07 shargon