nullable Storage.Get; refactor TokenContract #1214

Hecate2 · 2024-10-24T07:27:25Z

This PR is a bit risky and may affect existing contract codes. I do not mind abandoning it if not good.
Storage.Get may actually return null. Ignoring it without generating any warning for the contract developer is not good.
Also it is not the best to blindly translate null to BigInteger 0. And it costs GAS to check whether a ByteString is null, when converting it to BigInteger.
Judging null increases contract size, but can be optimized by #1213 .

Jim8y · 2024-10-24T07:40:55Z

This will not affect existing contract, unless they compile it again.... so, not really a big deal, we can add a warning to the contract when they build it. But the behavior should be properly commented. Such that developers can see how it works when they use it.

shargon · 2024-10-24T09:17:13Z

src/Neo.SmartContract.Framework/ByteString.cs

@@ -54,11 +54,6 @@ IEnumerator IEnumerable.GetEnumerator()
        [OpCode(OpCode.CONVERT, StackItemType.ByteString)]
        public static extern explicit operator ByteString(byte[] buffer);

-        [OpCode(OpCode.DUP)]


I prefer to avoid this change for the moment, only nullables

…to nullable-get # Conflicts: # tests/Neo.SmartContract.Template.UnitTests/templates/neocontractnep17/TestingArtifacts/Nep17ContractTemplate.artifacts.cs

Hecate2 · 2024-10-25T16:23:37Z

NUMEQUAL can be used in TokenContract

…to nullable-get

…into nullable-get

shargon · 2024-10-26T08:08:42Z

src/Neo.SmartContract.Framework/Services/StorageMap.cs

@@ -112,7 +112,9 @@ public extern ByteString this[byte[] key]
        [OpCode(OpCode.CAT)]
        [OpCode(OpCode.SWAP)]
        [Syscall("System.Storage.Get")]
-        public extern ByteString Get(ByteString key);
+#pragma warning disable CS8632 // The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.


Enable nullable at the beginning of the file?

OK. Maybe I shall just delete the pragma comments

src/Neo.SmartContract.Framework/TokenContract.cs

shargon · 2024-10-26T08:11:36Z

src/Neo.SmartContract.Framework/UInt256.cs

@@ -27,11 +27,13 @@ public extern bool IsValid
        {
            [OpCode(OpCode.DUP)]
            [OpCode(OpCode.ISTYPE, "0x28")] //ByteString
-            [OpCode(OpCode.SWAP)]
+            [OpCode(OpCode.JMPIF, "06")]  // to SIZE


This is cheaper?

Yes, and better for NZ/ISNULL and folding NOT in optimization

Also this avoids executing SIZE on a possibly null input, and further saves if (input is null) before using isValid

This should be in a different PR

Jim8y · 2024-10-27T07:08:50Z

src/Neo.SmartContract.Template/templates/neocontractnep17/Nep17Contract.cs

@@ -60,14 +60,14 @@ public static void SetOwner(UInt160 newOwner)

        public static new void Burn(UInt160 account, BigInteger amount)
        {
-            if (IsOwner() == false)


actually == false is a better format as it is reader friendly, but anyway. i dont mind.

# Conflicts: # tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Linq.cs # tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_NEP11.cs # tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_NEP17.cs # tests/Neo.SmartContract.Framework.UnitTests/TestingArtifacts/Contract_SupportedStandard11Enum.cs # tests/Neo.SmartContract.Framework.UnitTests/TestingArtifacts/Contract_SupportedStandard17Enum.cs # tests/Neo.SmartContract.Template.UnitTests/templates/neocontractnep17/TestingArtifacts/Nep17ContractTemplate.artifacts.cs # tests/Neo.SmartContract.Template.UnitTests/templates/neocontractowner/TestingArtifacts/OwnableTemplate.artifacts.cs

# Conflicts: # tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Linq.cs # tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_NEP11.cs # tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_NEP17.cs # tests/Neo.Compiler.CSharp.UnitTests/TestingArtifacts/Contract_Pattern.cs # tests/Neo.SmartContract.Framework.UnitTests/TestingArtifacts/Contract_SupportedStandard11Enum.cs # tests/Neo.SmartContract.Framework.UnitTests/TestingArtifacts/Contract_SupportedStandard17Enum.cs # tests/Neo.SmartContract.Template.UnitTests/templates/neocontractnep17/TestingArtifacts/Nep17ContractTemplate.artifacts.cs # tests/Neo.SmartContract.Template.UnitTests/templates/neocontractowner/TestingArtifacts/OwnableTemplate.artifacts.cs

Jim8y · 2024-11-08T06:10:23Z

@shargon please review

shargon · 2024-11-08T07:43:36Z

tests/Neo.Compiler.CSharp.UnitTests/UnitTest_UIntTypes.cs

-            AssertGasConsumed(1047330);
+            AssertGasConsumed(1047450);
+            Assert.IsFalse(Contract.ValidateAddress(InvalidUInt160.Null));
+            AssertGasConsumed(1047450);


There are cases where the cost is greater

No greater.
For invalid input,
1047930 -> 1047450
For null input,
Exception with 1047330 -> no exception with 1047450

shargon

I prefer to move the uint optimizations in a different PR

Hecate2 · 2024-11-08T07:54:15Z

I prefer to move the uint optimizations in a different PR

Ok I am on it. is null will be still used in TokenContract for this PR.

nullable Storage.Get

f25d26d

Hecate2 force-pushed the nullable-get branch from 8df1f98 to f25d26d Compare October 24, 2024 07:28

shargon reviewed Oct 24, 2024

View reviewed changes

revert operator BigInteger(ByteString

dc46908

Hecate2 force-pushed the nullable-get branch from 558658f to dc46908 Compare October 25, 2024 06:30

Merge branch 'master' of github.com:neo-project/neo-devpack-dotnet in…

4faf6ed

…to nullable-get # Conflicts: # tests/Neo.SmartContract.Template.UnitTests/templates/neocontractnep17/TestingArtifacts/Nep17ContractTemplate.artifacts.cs

Hecate2 marked this pull request as ready for review October 25, 2024 06:33

Merge branch 'master' into nullable-get

903fb62

Jim8y requested a review from shargon October 25, 2024 15:55

Hecate2 added 5 commits October 26, 2024 00:33

Merge branch 'master' of github.com:neo-project/neo-devpack-dotnet in…

5bac102

…to nullable-get

optimize framework contracts

8d6ccba

Merge branch 'nullable-get' of github.com:Hecate2/neo-devpack-dotnet …

141f507

…into nullable-get

optimize IsValid for UInt160 & UInt256

36d7715

cancel is null with isValid

e030734

shargon reviewed Oct 26, 2024

View reviewed changes

Hecate2 and others added 5 commits October 26, 2024 16:35

cancel unnecessary pragma annotation

479f63e

Merge branch 'master' into pr/1214

1c31a70

update artifacts

0fbd2ac

update gas

0672337

update last gas

33ae8d4

Jim8y requested a review from shargon October 27, 2024 07:06

Jim8y reviewed Oct 27, 2024

View reviewed changes

Jim8y added 3 commits October 27, 2024 19:00

Merge branch 'master' into pr/1214

067b70a

shargon reviewed Nov 8, 2024

View reviewed changes

Hecate2 force-pushed the nullable-get branch from 547f443 to 55e9145 Compare November 8, 2024 08:08

revert IsValid

34926a0

Hecate2 force-pushed the nullable-get branch from 55e9145 to 34926a0 Compare November 8, 2024 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nullable Storage.Get; refactor TokenContract #1214

nullable Storage.Get; refactor TokenContract #1214

Hecate2 commented Oct 24, 2024 •

edited

Loading

Jim8y commented Oct 24, 2024 •

edited

Loading

shargon Oct 24, 2024

Hecate2 commented Oct 25, 2024

shargon Oct 26, 2024

Hecate2 Oct 26, 2024

shargon Oct 26, 2024

Hecate2 Oct 26, 2024

Hecate2 Oct 26, 2024

shargon Oct 26, 2024

Jim8y Oct 27, 2024 •

edited

Loading

Jim8y commented Nov 8, 2024

shargon Nov 8, 2024

Hecate2 Nov 8, 2024

shargon left a comment

Hecate2 commented Nov 8, 2024

nullable Storage.Get; refactor TokenContract #1214

Are you sure you want to change the base?

nullable Storage.Get; refactor TokenContract #1214

Conversation

Hecate2 commented Oct 24, 2024 • edited Loading

Jim8y commented Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Hecate2 commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y Oct 27, 2024 • edited Loading

Choose a reason for hiding this comment

Jim8y commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon left a comment

Choose a reason for hiding this comment

Hecate2 commented Nov 8, 2024

Hecate2 commented Oct 24, 2024 •

edited

Loading

Jim8y commented Oct 24, 2024 •

edited

Loading

Jim8y Oct 27, 2024 •

edited

Loading