Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: Fix default value when Stored #895

Merged
merged 13 commits into from
Feb 14, 2024
Merged

bug: Fix default value when Stored #895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
be7dfec
Fix default value when Stored
shargon Feb 13, 2024
04e252e
Fix Default value
shargon Feb 13, 2024
32a14c8
Fix
shargon Feb 13, 2024
a32baf5
add Int16
shargon Feb 13, 2024
f47eed2
Fix ut
shargon Feb 13, 2024
8f722ad
Improve reading
shargon Feb 13, 2024
26db4e6
Merge branch 'master' into core-store-default
shargon Feb 13, 2024
63c1f13
Merge branch 'master' into core-store-default
Jim8y Feb 13, 2024
b454f74
Apply suggestions from code review
shargon Feb 13, 2024
74e7b72
added types
shargon Feb 14, 2024
edcd9fa
Breaking changes reporting during releases
shargon Feb 14, 2024
e8f42a5
Delete .github/workflows/release.yml
shargon Feb 14, 2024
6e258f4
Merge branch 'master' into core-store-default
shargon Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 42 additions & 9 deletions src/Neo.Compiler.CSharp/MethodConvert/MethodConvert.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,21 +574,42 @@ private void ConvertStorageBackedProperty(IPropertySymbol property, AttributeDat
Call(ApplicationEngine.System_Storage_Get);
switch (property.Type.Name)
{
case "SByte":
case "Short":
case "Int32":
case "Int64":
case "byte":
case "sbyte":
case "Byte":
case "SByte":

case "short":
case "ushort":
case "Int16":
case "UInt16":

case "int":
case "uint":
case "Int32":
case "UInt32":

case "long":
case "ulong":
case "Int64":
case "UInt64":
case "BigInteger":
ChangeType(VM.Types.StackItemType.Integer);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the bug
https://github.com/neo-project/neo/blob/4a5a7f58567c1bf270d1c5515d90486c803b4e11/src/Neo.VM/Types/Null.cs#L30

// Replace NULL with 0
AddInstruction(OpCode.DUP);
AddInstruction(OpCode.ISNULL);
JumpTarget ifFalse = new();
Jump(OpCode.JMPIFNOT_L, ifFalse);
{
AddInstruction(OpCode.DROP);
AddInstruction(OpCode.PUSH0);
}
ifFalse.Instruction = AddInstruction(OpCode.NOP);
break;
case "String":
case "ByteString":
case "UInt160":
case "UInt256":
case "ECPoint":
break;
default:
Call(NativeContract.StdLib.Hash, "deserialize", 1, true);
Expand Down Expand Up @@ -620,19 +641,31 @@ private void ConvertStorageBackedProperty(IPropertySymbol property, AttributeDat
AccessSlot(OpCode.LDARG, 1);
switch (property.Type.Name)
{
case "SByte":
case "Short":
case "Int32":
case "Int64":
case "byte":
case "sbyte":
case "Byte":
case "SByte":

case "short":
case "ushort":
case "Int16":
case "UInt16":

case "int":
case "uint":
case "Int32":
case "UInt32":

case "long":
case "ulong":
case "Int64":
case "UInt64":
Copy link
Contributor

Choose a reason for hiding this comment

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

Not an issue here, but should we check the length of UInt160, UInt256 and ECPoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

the unique way to set a different value in a property is because an storage collision (same prefix) and manual write to the same storage

case "BigInteger":
case "String":
case "ByteString":
case "UInt160":
case "UInt256":
case "ECPoint":
Copy link

Choose a reason for hiding this comment

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

This change can break existing contracts on upgrade IIUC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it's wrong, I think that we don't have any release with Stored attribute

Copy link
Member Author

Choose a reason for hiding this comment

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

The user should review the breaking changes #900 until a more stable version

Copy link

Choose a reason for hiding this comment

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

Ah, yes, it was added in #841 and there were no releases since November, so it should be OK changing it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This change can break existing contracts on upgrade IIUC.

Stored attribute is still not released. Won't cause any trouble.

break;
default:
Call(NativeContract.StdLib.Hash, "serialize", 1, true);
Expand Down
6 changes: 2 additions & 4 deletions tests/Neo.SmartContract.Framework.TestContracts/Contract_Stored.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
using System.Numerics;
using Neo.SmartContract.Framework;
using Neo.SmartContract.Framework.Attributes;
using Neo.SmartContract.Framework.Services;
using System.Numerics;

namespace Neo.SmartContract.Framework.UnitTests.TestClasses
{
public class Contract_StorageBacked : SmartContract
public class Contract_Stored : SmartContract
{
// Test non-static

Expand Down
10 changes: 4 additions & 6 deletions tests/Neo.SmartContract.Framework.UnitTests/Services/BackedStorageTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ public void Test_Private_Getter_Public_Setter()

var result = _engine.ExecuteTestCaseStandard("getPrivateGetterPublicSetter");
Assert.AreEqual(VMState.HALT, _engine.State);
Assert.IsTrue(result.Pop().IsNull);

Assert.AreEqual(0, result.Pop());

// Test public setter
_engine.Reset();
Expand Down Expand Up @@ -81,8 +80,7 @@ public void Test_Non_Static_Private_Getter_Public_Setter()

var result = _engine.ExecuteTestCaseStandard("getNonStaticPrivateGetterPublicSetter");
Assert.AreEqual(VMState.HALT, _engine.State);
Assert.IsTrue(result.Pop().IsNull);

Assert.AreEqual(0, result.Pop());

// Test public setter
_engine.Reset();
Expand All @@ -108,14 +106,14 @@ public void Test_Kind(string kind)

var result = _engine.ExecuteTestCaseStandard("get" + kind);
Assert.AreEqual(VMState.HALT, _engine.State);
Assert.IsTrue(result.Pop().IsNull);
Assert.AreEqual(0, result.Pop());

// Test public getter

_engine.Reset();
result = _engine.ExecuteTestCaseStandard(kind[0].ToString().ToLowerInvariant() + kind[1..]);
Assert.AreEqual(VMState.HALT, _engine.State);
Assert.IsTrue(result.Pop().IsNull);
Assert.AreEqual(0, result.Pop());

// Put

Expand Down