BoomDAO / ICP.NET

A library for .NET/C#/Unity to natively communicate with the Internet Computer (ICP)
MIT License
50 stars 3 forks source link

Replace Sysem.Object? Value in Variants to an Interface. #38

Closed LorenzGit closed 1 year ago

LorenzGit commented 1 year ago

Could it be possible to replace the "System.Object? Value" with a "IMyVariant Value" instead? And every variant type to implement that type?

The IMyVariant would be empty, it would literally be used simply as an interface identifier. So that way the validation of the proper Object is built in at the compiler level. Currently you can put anything you want in Value, because it's an Object. Having it be IMyVariant would allow for compiler validation.

This also require that the classes MyVariantOne and MyVariantTwo will need to be generated as MyVariantOne : IMyVariant and MyVariantTwo : IMyVariant

Gekctek commented 1 year ago

@LorenzGit Im trying to figure out how to make this generic enough to make sense for more than this case Im assuming this is a Unity issue, what exactly are you running into

Also im confused by the second part, would it look like this?

public class MyVariantOne : IMyVariant
{
...
    public IMyVariant Value { get; }
...
}

Also how would this work for enum variants?

LorenzGit commented 1 year ago

Currently Variants are written like this:

[Variant(typeof(ContentsTag))]
public class Contents
{
    [VariantTagProperty()]
    public ContentsTag Tag { get; set; }

    [VariantValueProperty()]
    public System.Object? Value { get; set; }

    public Contents(ContentsTag tag, object? value)
    {
        this.Tag = tag;
        this.Value = value;
    }

    protected Contents()
    {
    }   

    public static Contents FirstClass(FirstClass info)
    {
        return new Contents(ContentsTag.FirstClass, info);
    }   

    public static Contents SecondClass(SecondClass info)
    {
        return new Contents(ContentsTag.SecondClass, info);
    }

    public FirstClass AsFirstClass()
    {
        this.ValidateTag(ContentsTag.FirstClass);
        return (FirstClass)this.Value!;
    }

    public SecondClass AsSecondClass()
    {
        this.ValidateTag(ContentsTag.SecondClass);
        return (SecondClass)this.Value!;
    }

    private void ValidateTag(ContentsTag tag)
    {
        if (!this.Tag.Equals(tag))
        {
            throw new InvalidOperatiFirstClassxception($"Cannot cast '{this.Tag}' to type '{tag}'");
        }
    }
}

public enum ContentsTag
{   
    [VariantOptionType(typeof(FirstClass))]
    FirstClass,
    [VariantOptionType(typeof(SecondClass))]
    SecondClass,
}

public class FirstClass
{
    public OptionalValue<int> rarity { get; set; }

    public FirstClass(OptionalValue<int> rarity)
    {
        this.rarity = rarity;
    }

    public FirstClass()
    {
    }
}

public class SecondClass
{
    public OptionalValue<ushort> quantity { get; set; }

    public SecondClass(OptionalValue<ushort> quantity)
    {
        this.quantity = quantity;
    }

    public SecondClass()
    {
    }
}

What I am suggesting, is to use an interface rather than System.Object, something like this: essentially replacing Object with an Interface:

[Variant(typeof(ContentsTag))]
public class Contents
{
    [VariantTagProperty()]
    public ContentsTag Tag { get; set; }

    [VariantValueProperty()]
    public IContentsVariant? Value { get; set; }

    public Contents(ContentsTag tag, IContentsVariant? value)
    {
        this.Tag = tag;
        this.Value = value;
    }

    protected Contents()
    {
    }   

    public static Contents FirstClass(FirstClass info)
    {
        return new Contents(ContentsTag.FirstClass, info);
    }   

    public static Contents SecondClass(SecondClass info)
    {
        return new Contents(ContentsTag.SecondClass, info);
    }

    public FirstClass AsFirstClass()
    {
        this.ValidateTag(ContentsTag.FirstClass);
        return (FirstClass)this.Value!;
    }

    public SecondClass AsSecondClass()
    {
        this.ValidateTag(ContentsTag.SecondClass);
        return (SecondClass)this.Value!;
    }

    private void ValidateTag(ContentsTag tag)
    {
        if (!this.Tag.Equals(tag))
        {
            throw new InvalidOperatiFirstClassxception($"Cannot cast '{this.Tag}' to type '{tag}'");
        }
    }
}

public enum ContentsTag
{   
    [VariantOptionType(typeof(FirstClass))]
    FirstClass,
    [VariantOptionType(typeof(SecondClass))]
    SecondClass,
}

public interface IContentsVariant{

}

public class FirstClass : IContentsVariant
{
    public OptionalValue<int> rarity { get; set; }

    public FirstClass(OptionalValue<int> rarity)
    {
        this.rarity = rarity;
    }

    public FirstClass()
    {
    }
}

public class SecondClass : IContentsVariant
{
    public OptionalValue<ushort> quantity { get; set; }

    public SecondClass(OptionalValue<ushort> quantity)
    {
        this.quantity = quantity;
    }

    public SecondClass()
    {
    }
}
Gekctek commented 1 year ago

@LorenzGit is there another way that we could approach this? This assumes that each option type is going to be a newly defined class. If its something like an int, List<T> or a reference to a different type declared in a file then we have a problem. You are welcome to do this my manually changing the generated types to match this, though i know that is tedious. My other thought, if we cant figure out an alternative is to have some custom hooks that can be injected into the client generator, but that would require a lot of work and for you to call the generator from code itself. What is the ultimate problem that you are running into with it being an Object? so I can maybe help brainstorm ideas for a solution

LorenzGit commented 1 year ago

@Gekctek what do you mean by "each option type is going to be a newly defined class", what is an example where it might not be that? Not sure I understand the example with int and List.

Gekctek commented 1 year ago

@LorenzGit if I have a candid definition that is

type Contents = variant {
  FirstClass : record { quantity : opt int32 }
  SecondClass : record { quantity : opt nat16 }
}

that might be ok since the inner types are going to be generated with the context of being in a variant But with something like

type OtherType = record { quantity : opt int32 }
type Contents = variant {
  FirstClass : OtherType
  SecondClass : record { quantity : opt nat16 }
}
type A = record {
  field1 : OtherType
}

Even though the variant stays the same, the inner type is not only used in other types but is no longer directly used in the variant context, which it makes it that much complicated to retroactively add the interface to the shared type. Its possible but since this feature seems hyper specific to your case, it doesn't seem the effort right now or would have to make tradeoffs for a feature that might not really be used by others. If we can figure out a more general solution or open it up to custom hooks for your own implementation, that would make more sense. I generally try not to create one off solutions for a general library What do you use the interface for?

LorenzGit commented 1 year ago

I see, that's interesting, I didn't think people would use a variant that way. If I understand what you are saying, someone could be putting a type in a variant that was not really meant to exist in that variant. Is this the exception or it's actually how everyone use variants?

Anyway, all I am trying to achieve with this request (similarly as the other request for the OptionalValue) is generate C# models that are as clear as possible to what they do, and could easily serialized/deserialized in other way. All it's missing is simply a compiler validation. Currently I can store anything I want in System.Object? Value, since it's an object type.

The current approach doesn't stop us from continuing development, it's just a little ambiguous because of the Object usage. Also, because Unity prefab serialization does not support System.Object we might have to do a post process pass as you suggested, by replacing those classes manually. We just have to figure out what is the easiest way to do a search and replace after the code as been generated.

Gekctek commented 1 year ago

Here is something from the IC governance canister KnownNeuron is used multiple places

type Action = variant {
  RegisterKnownNeuron : KnownNeuron;
  ManageNeuron : ManageNeuron;
  ExecuteNnsFunction : ExecuteNnsFunction;
  RewardNodeProvider : RewardNodeProvider;
  SetDefaultFollowees : SetDefaultFollowees;
  RewardNodeProviders : RewardNodeProviders;
  ManageNetworkEconomics : NetworkEconomics;
  ApproveGenesisKyc : ApproveGenesisKyc;
  AddOrRemoveNodeProvider : AddOrRemoveNodeProvider;
  Motion : Motion;
};

type KnownNeuron = record {
  id : opt NeuronId;
  known_neuron_data : opt KnownNeuronData;
};

type ListKnownNeuronsResponse = record { known_neurons : vec KnownNeuron };

I have done this kind of thing myself, just depends on the use case I guess

Ill have to think something for customization because im sure everyone to customize the client generator, but for the time being you might just be stuck with manual tweaking

Gekctek commented 1 year ago

Alright I made the Custom Client Generator more open to use: #65 See the README for the ClientGenerator about Custom Generators Maybe you can create a custom Rewriter to fix this issue?

https://github.com/edjCase/ICP.NET/releases/tag/2.2.7

Gekctek commented 1 year ago

Feel free to reopen if you need help