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

Jump to bottom

Avoid re-init properties #902

Closed

shargon wants to merge 4 commits into master from core-optimizer-init

Member

shargon commented May 7, 2024

Related to https://github.com/neo-project/neo-modules/pull/898/files#r1584389953


          Avoid re-init properties

e429386

cschuchardt88 reviewed

View reviewed changes

Member

cschuchardt88 left a comment •

edited

Loading

You need these or else you have to initialize each one, because they will be null or no instance when you do new(); ISerializable.Serialize or ISerializable.Deserialize will get exception object not set to an instance of an object. Also on some of them Size won't work because of that reason.

Member Author

shargon commented May 7, 2024

you have to initialize each one

Are always initialized, isn't it?

Member

cschuchardt88 commented May 7, 2024 •

edited

Loading

No, you would have to do something like this:

var obj = new AClass()
{
  AProperty = new(),
}

vncoelho reviewed

View reviewed changes

src/ApplicationLogs/Store/States/TransactionLogState.cs

                       public virtual void Deserialize(ref MemoryReader reader)
                       {
                           // It should be safe because it filled from a transaction's notifications.
-                          uint aLen = reader.ReadUInt32();
+                          var aLen = reader.ReadUInt32();

Member

vncoelho May 7, 2024

Is this related?

Member

cschuchardt88 May 7, 2024

No, it's just using dynamic type variable now; instead of being bound to a type specific variable.

Member Author

shargon commented May 7, 2024

No, you would have to do something like this:
var obj = new AClass()
{
  AProperty = new(),
}

It's currently done, otherwise you will have a default unused value, that will be changed in Create method

Member

cschuchardt88 commented May 7, 2024 •

edited

Loading

Yes, But if someone was do bytes.AsSerializable<AClass>() it would throw that exception. This library was importable for other plugins to access the store.

Lets have @superboyiii test 1st. before merge.

Member Author

shargon commented May 8, 2024

Yes, But if someone was do bytes.AsSerializable<AClass>() it would throw that exception. This library was importable for other plugins to access the store.

Lets have @superboyiii test 1st. before merge.

Good point, but it call Deserialize


          Merge branch 'master' into core-optimizer-init

8a91fe8

cschuchardt88 requested changes

View reviewed changes

src/ApplicationLogs/Store/States/NotifyLogState.cs

-                      public Guid[] StackItemIds { get; protected set; } = [];
+                      public UInt160 ScriptHash { get; protected set; }
+                      public string EventName { get; protected set; }
+                      public Guid[] StackItemIds { get; protected set; }

Member

cschuchardt88 May 8, 2024

We should make this required as well.

src/ApplicationLogs/Store/States/TransactionEngineLogState.cs

@@ @@ -15,9 +15,9 @@ namespace Neo.Plugins.Store.States @@
               {
                   public class TransactionEngineLogState : ISerializable, IEquatable<TransactionEngineLogState>
                   {
-                      public Guid[] LogIds { get; private set; } = Array.Empty<Guid>();
+                      public Guid[] LogIds { get; private set; }

Member

cschuchardt88 May 8, 2024

We should make this required as well.

Member Author

shargon May 9, 2024

Then set can't be private

Member

cschuchardt88 May 9, 2024 •

edited

Loading

They have Create method and that should be used. Plus you still run into the same problem with null. So you need to add or remove changes on these files. I marked in review.

src/ApplicationLogs/Store/States/TransactionLogState.cs

@@ @@ -16,9 +16,9 @@ namespace ApplicationLogs.Store.States @@
               {
                   public class TransactionLogState : ISerializable, IEquatable<TransactionLogState>
                   {
-                      public Guid[] NotifyLogIds { get; private set; } = Array.Empty<Guid>();
+                      public Guid[] NotifyLogIds { get; private set; }

Member

cschuchardt88 May 8, 2024

We should make this required as well.

src/ApplicationLogs/Store/States/ExecutionLogState.cs

+                      public VMState VmState { get; private set; }
+                      public string Exception { get; private set; }
+                      public long GasConsumed { get; private set; }
+                      public Guid[] StackItemIds { get; private set; }

Member

cschuchardt88 May 8, 2024

We should make this required as well.

src/ApplicationLogs/Store/States/EngineLogState.cs

-                      public UInt160 ScriptHash { get; private set; } = new();
-                      public string Message { get; private set; } = string.Empty;
+                      public UInt160 ScriptHash { get; private set; }
+                      public string Message { get; private set; }

Member

cschuchardt88 May 8, 2024

We should make this required as well.

src/ApplicationLogs/Store/States/ContractLogState.cs

-                      public UInt256 TransactionHash { get; private set; } = new();
-                      public TriggerType Trigger { get; private set; } = TriggerType.All;
+                      public UInt256 TransactionHash { get; private set; }
+                      public TriggerType Trigger { get; private set; }

Member

cschuchardt88 May 8, 2024

We should make this required as well.

src/ApplicationLogs/Store/States/BlockLogState.cs

@@ @@ -16,9 +16,9 @@ namespace ApplicationLogs.Store.States @@
               {
                   public class BlockLogState : ISerializable, IEquatable<BlockLogState>
                   {
-                      public Guid[] NotifyLogIds { get; private set; } = [];
+                      public Guid[] NotifyLogIds { get; private set; }

Member

cschuchardt88 May 8, 2024

We should make this required as well.

Contributor

Jim8y May 10, 2024

like this? Kinda ugly:

public virtual void Deserialize(ref MemoryReader reader)
{
    // It should be safe because it filled from a block's notifications.
    var aLen = reader.ReadUInt32();
    var notifyLogIds = new Guid[aLen];
    for (var i = 0; i < aLen; i++)
        notifyLogIds[i] = new Guid(reader.ReadVarMemory().Span);

    // Create a new instance of BlockLogState using the Create factory method
    var blockLogState = Create(notifyLogIds);

    // Assign the created instance to the current object
    NotifyLogIds = blockLogState.NotifyLogIds;
}

Jim8y added 2 commits

May 10, 2024 18:27


          Merge branch 'master' into core-optimizer-init

15aca16


          Merge branch 'master' into core-optimizer-init

Member Author

shargon commented May 21, 2024

Repository moved to neo, please re-open there

shargon closed this

shargon deleted the core-optimizer-init branch

May 21, 2024 08:04

shargon mentioned this pull request

Optimize plugin's models neo-project/neo#3246

Merged

10 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet