massalabs / massa-as-sdk

MIT License
13 stars 5 forks source link

Be able to specify the type of the object we will get in return of the get function, whatever the type of the key #136

Open Thykof opened 1 year ago

Thykof commented 1 year ago

Context Describe / explain why we should do this: motivations, context or other info. Keep it brief and simple - PM

Improve developer experience

User flow Describe the user flow using user stories so the end result is super clear - PM

Storage.get<Args>('string_as_key'); // get an args in return

And in a second step: (to confirm the need) const amount: Amount = Storage.get<Amount>('key');

How to List the step-by-step to get it do if needed - PM

Technical details Give the technical insights so anyone in the team can tackle the tasks - Dev

First batch of types we want to implement:

QA testing Does this task require some QA tests? If yes, explain how to validate it

We must write the unit tests that cover all cases.

Ben-Rey commented 1 year ago

I thought of 3 solutions but maybe I miss something

Solution 1


export function get<T, R>(key: T): R {
  const value: StaticArray<u8> = env.get(toDatastoreFormat(key));

  return fromDatastoreFormat<R>(value);
}

// We must specify the generics type when calling the function
Storage.get<String, Address>("myKey")

Problem: We are not able to call the function without specifying the generics types so if we write get("myKey) we will get an error. This would be a breaking change

This does not work

export function test<T, R = T>(key: T): R {
  log(sizeof<T>());
  const value: StaticArray<u8> = env.get(toDatastoreFormat(key));

  return fromDatastoreFormat<R>(value);
}

Solution 2

// We don't change the original function
export function get<T>(key: T): T {
  const value: StaticArray<u8> = env.get(toDatastoreFormat(key));

  return fromDatastoreFormat<T>(value);
}

// We write new functions for each possible return type
export function getArgs<T>(key: T): Args {
  const value: StaticArray<u8> = env.get(toDatastoreFormat(key));

  return fromDatastoreFormat<Args>(value);
}

export function getStaticArrayU8<T>(key: T): StaticArray<u8> {
  const value: StaticArray<u8> = env.get(toDatastoreFormat(key));

  return fromDatastoreFormat<StaticArray<u8>>(value);
}

export function getString(key: T): String {
  const value: StaticArray<u8> = env.get(toDatastoreFormat(key));

  return fromDatastoreFormat<String>(value);
}

No breaking change but code repetition

Solution 3

export function get<T, R = string>(key: T): R {
  log(sizeof<T>());
  const value: StaticArray<u8> = env.get(toDatastoreFormat(key));

  return fromDatastoreFormat<R>(value);
}

In that case get("myKey") still works but we assume that the return type is a string so if we have get(new Args().add("Something")) and we expect an Arg in return, the code will break. We would have to write instead Storage.get<Args, Args>("myKey")

Thykof commented 1 year ago

Thank you for investigating and proposing alternatives!

I would prefer solution 2 to avoid breaking changes and have a cleaner code.

Let's ask some other people what they thing @massalabs/innovation

peterjah commented 1 year ago

Imho, as we are still in testnet, i don't really care about breaking changes if its bringing cool features and/or clean code. Another thing: I dont understand the added value of having several types for the datastore key. I key is a key, the type doesnt really matter. so i thing we are trying to do useless generic stuff here that brings complexity.

I don't know if it can help, but here is a nice exemple of use of AS type templating: https://github.com/JairusSW/as-tbs/blob/master/assembly/src/tbs.ts

Ben-Rey commented 1 year ago

I agree with both of Pierre's remarks in case the new feature brings a real added value. It would be nice to have opinions on the generic type of the key. Is it used ? I'll take a look at as-tbs

gregLibert commented 1 year ago

Another thing: I dont understand the added value of having several types for the datastore key. I key is a key, the type doesnt really matter. so i thing we are trying to do useless generic stuff here that brings complexity.

So what type would you use for the key ? StaticArray? Which one is simpler ?

Thykof commented 1 year ago

simpler one is String imo

damip commented 1 year ago

simpler one is String imo

I'm personnally not a big fan of requiring strings as keys because the "normal" use case is to use bytes. For example if I want to create an ERC20, I'm better off encoding address keys directly as bytes because:

That being said, I didn't study this issue as much as you, so take my opinion with a grain of salt, it's just that as a developer I feel like restraining the key type to string is very unusual and feels clunky.

Thykof commented 1 year ago

At the beginning, we added type parameter to avoid doing the conversion in the smart contract code itself.

Storage.set(args); // I get an args in return

The live presentation about this brought this idea: be able to specify the return type. because some time we do new Args(Storage.get('key_as_string')); or Storage.get(new Args().add('key_as_string')); // get an args in return.

So in this issue we would like do to Storage.get<Args>('string_as_key'); // get an args in return.

This issue is about the return type of Storage.get.

If we want to change the rules about the key type of the storage, don't hesitate to create a new issue.

If we are mistaking, solution 2 proposed by Ben is the less risky because it only add 3 functions. It doesn't matter if none use them, we don't break existing code. For these reason I prefer solution 2.

If some of you prefer to close this issue without changing anything, why not.

damip commented 1 year ago

ve presentation about this brought this idea: be able to specify the return type. because some time w

I agree with the idea, I just wanted to make sure that the key is stored on the ledger as binary and not needlessly converted to strings when it is not necessary (eg. for numbers, addresses etc...)

Ben-Rey commented 1 year ago

Thank you all for this discussion which will help me a lot. I will try to talk to some builders to get a better idea of how they would like to use this feature. Then I will come back with a version that takes your comments into account.

gregLibert commented 1 year ago

@Ben-Rey wherre are we on this subject? After reviewing the storage package, I realy think that the key should only be of type StaticArray or String. Other types just add useless complexity.

Ben-Rey commented 1 year ago

I have to sync with @BatiGencho