Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

ByteArray SETITEM fails #58

Closed
igormcoelho opened this issue Oct 3, 2018 · 2 comments
Closed

ByteArray SETITEM fails #58

igormcoelho opened this issue Oct 3, 2018 · 2 comments

Comments

@igormcoelho
Copy link
Contributor

People, I don't know if it's something we can improve on neo-vm, or here at the compiler, but I really think this code should work:

using Neo.SmartContract.Framework;
using Neo.SmartContract.Framework.Services.Neo;
using System;
using System.Numerics;
namespace NeoContract1   {
    public class Contract1 : SmartContract  {
        public static byte[] Main()    {
            byte[] b = new byte[]{0x01, 0x02, 0x03, 0x04};
            b[2] = 0x05;
            return b;
        }
    }
}

I generates the following AVM 00c56b0401020304765255c4616c7566, that means:

00 PUSH0  #An empty array of bytes is pushed onto the stack
c5 NEWARRAY  #
6b TOALTSTACK  # Puts the input onto the top of the alt stack. Removes it from the main stack.
04 PUSHBYTES4 01020304 # 
76 DUP  # Duplicates the top stack item.
52 PUSH2  # The number 2 is pushed onto the stack.
55 PUSH5  # The number 5 is pushed onto the stack.
c4 SETITEM  # bytearray[2] = 5
61 NOP  # Does nothing.
6c FROMALTSTACK  # Puts the input onto the top of the main stack. Removes it from the alt stack.
75 DROP  # Removes the top stack item.
66 RET  #

Operation SETITEM naturally fails, because it's only currently allowed for arrays or maps (not bytearray). I think it should be allowed on bytearrays, since the alternative to this is to do lots of substrings and concats (like this ugly solution: https://github.com/NeoResearch/learning-examples/blob/master/NonTrivialFunctions.md).

The problem I see is that it is applying a DUP because it's guessing ByteArray is a reference type, so collateral effects should apply "remotely", but that won't work on copy type bytearray. So, how can we fix this in a easier way? Allow SETITEM to update bytearray too (on neo-vm project) and make the compiler avoid the DUP in these cases (in this project)?

========

Just to mention that this code works:

            object[] obj = new object[]{0x01, 0x02, 0x03, 0x04};
            obj[2] = 0x05;

But since it works as native array, it pushes elements one by one, and it's quite hard to convert it to byte[] (requires a full loop and many concats). So, a direct solution for byte[] would be nice, perhaps avoiding generating the DUP and allowing SETITEM to push byte[] back to stack after updating it.

Moved from: https://github.com/neo-project/neo-compiler/issues/133

@igormcoelho
Copy link
Contributor Author

@erikzhang This will have the same problem as #57 , because SETITEM will also need to push the output in this situation, differently from others involving Array and Map. Perhaps the best solution for both is to have an operation that converts from ByteArray to Array (of byte) and back. So, before REVERSE or SETITEM on bytearrays, we just use this operation to convert between types... perhaps CONVERT opcode? Or interop? What do you think @shargon?

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Oct 3, 2018

I'm abandoning this issue in favor of this one, I think it's a better solution: #59

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant