Fixed GetString method on VM Types

[cschuchardt88]

Description

The Integer, Boolean type in the VM would use the default binary bytes of the BigInteger to output the string. My guess is they forgot to override the method in Integer.cs and Boolean.cs.

Now outputs the value of the integer. For example if Integer is 1 then it outputs 1 as a string.

Change Log

• Fixed GetString method on VM Integer and Boolean type

Type of change

• Optimization (the change is only an optimization)
• Style (the change is only a code style for better maintenance or standard purpose)
• 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)
• This change requires a documentation update

How Has This Been Tested?

• Locally

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[cschuchardt88]

@shargon @Jim8y @AnnaShaleva would this need to be a hardfork?

[Jim8y]

do we need it?

[shargon]

do we need it?

It require a hardfork, and we have syscall for the conversion, we don't need it in my opinion

[cschuchardt88]

do we need it?

It require a hardfork, and we have syscall for the conversion, we don't need it in my opinion

But currently its returning invalid string as ByteString instead. Why not fix it. Because if we do value.GetString() it will return right value for BigInteger

For example (Currently)

var i = (Integer)BigInteger.One;
var str = i.GetString(); // Output is "\x01" not "1"

[AnnaShaleva]

Why not fix it.

If it causes a hardfork, then we can live without this fix, to me it just doesn't worth the hardfork.

[cschuchardt88]

What's the big deal with it being in the next hardfork? You want to keep invalid or bugged data on chain, than be my guess. So I guess i should stop fixing the vm, since I can't get anywhere? Give me one good reason, besides you don't want too?

[shargon]

It can be fixed in the compiler

[cschuchardt88]

What about a plugins? or core itself. Compiler can't help there.

[shargon]

If GetString returns binary data with an Integer, compiler can fix it

[cschuchardt88]

But doesn't the vm use StackItem.GetString() for that syscall? This broken for boolean too.

Also if i do ApplicationEngine and pop then do GetString it wont work. unless we use this PR

Example:

            using var engine = ApplicationEngine.Run(script, _dataCache, settings: neoSystem.Settings, gas: RestServerSettings.Current.MaxGasInvoke);
            if (engine.State != VMState.HALT)
                throw new NotSupportedException(nameof(scriptHash));
            var boolstr = engine.ResultStack.Pop().GetString() ?? string.Empty; // This is a boolean or interger

[roman-khimov]

invalid or bugged data on chain

It's not invalid. It's not bugged. It's just that this behavior doesn't meet your expectations. But this behavior is known and it exists for 3+ years of N3, there is a lot of code that is written relying on this behavior. This change will just break it. For what purpose? Does this change solve any problem that can't be solved in another way? No, int/string conversion can be handled otherwise. Can we live without it? Yes, easily. So why making this change?

[cschuchardt88]

Everyone wants to keep the behavior of not allowing data types to be converted to string properly for the vm. Instead they insist on users using the native contract to convert integers to base2.