Concordium / concordium-dapp-libraries

A coherent set of building blocks for making it as easy as possible for developers to build web-based dApps.
Apache License 2.0
7 stars 5 forks source link

wallet-connectors: Support encoding parameters using all schema types #34

Closed bisgardo closed 1 year ago

bisgardo commented 1 year ago

Purpose

Allow smart contract invocation parameters provided to WalletConnection.signAndSendTransaction(...) to be encoded using parameter schemas rather than only full module schemas.

This also fixes the current inability to invoke contract methods that don't take parameters.

Changes

Replace the parameters and schemaVersion parameters of signAndSendTransaction with a new union type Schema.

The interface requirements for the method was also tightened up and made consistent between both implementations.

This PR is an alternative to #33.

bisgardo commented 1 year ago

I would think this should be backwards compatible? (I don't really see why this is better than my solution, you've just changed the field names and made a breaking change for legacy schemas thinking)

I don't see the need for strict backwards compatibility here. It's still a new, pre-1.0 library and this it's trivial for the user to wrap there schema string in a function call. I would rather get the interface right for 1.0 and I have more breaking changes besides this in mind. Then I'll start trying to avoid changing it.

I really care about interfaces, and think including literal string just for compatibility back to the earliest versions is not worth it.

I didn't just change the fields names, I also moved schema version into the type and fixed encoding for walletconnect. I generally think it's a mistake to use the types from the browser wallet API in this interface, because they should be allowed to evolve independently.

shjortConcordium commented 1 year ago

I meant the only difference in the interface was changing the field names, and the moving the schema version (which is a breaking change for only legacy schemas).

I guess it's fine to do the schema version change, but I don't see the point in changing the type field to kind, instead of aligning between the BW and the library? :thinking:

bisgardo commented 1 year ago

I guess it's fine to do the schema version change, but I don't see the point in changing the type field to kind, instead of aligning between the BW and the library? thinking

It's not the same field: The type in BW API is an enum SchemaType which is a property of SchemaWithContext. Here, Schema is a discriminated union, and from reading up on it it looks to me like kind is the conventional name used for the discriminator field. I also considered using class types in which case the field wouldn't be there at all.

Besides, the user is supposed to create the objects use the constructor functions, not manually, so they aren't going to interact with this field in any way.

shjortConcordium commented 1 year ago

It's not the same field: The type in BW API is an enum SchemaType which is a property of SchemaWithContext. Here, Schema is a discriminated union, and from reading up on it it looks to me like kind is the conventional name used for the discriminator field. I also considered using class types in which case the field wouldn't be there at all.

Besides, the user is supposed to create the objects use the constructor functions, not manually, so they aren't going to interact with this field in any way.

Okay, I really don't care, so we can just do it your way, but it is the same, the tagging is just more explicit now, because you've added the version that needs to be "discriminated". I've seen tag / type / kind used interchangeably for tagging like this. And if you used type instead of kind, it would align with the browser wallet for users who create it themselves. :upside_down_face:

bisgardo commented 1 year ago

It's not the same field: The type in BW API is an enum SchemaType which is a property of SchemaWithContext. Here, Schema is a discriminated union, and from reading up on it it looks to me like kind is the conventional name used for the discriminator field. I also considered using class types in which case the field wouldn't be there at all. Besides, the user is supposed to create the objects use the constructor functions, not manually, so they aren't going to interact with this field in any way.

Okay, I really don't care, so we can just do it your way, but it is the same, the tagging is just more explicit now, because you've added the version that needs to be "discriminated". I've seen tag / type / kind used interchangeably for tagging like this. And if you used type instead of kind, it would align with the browser wallet for users who create it themselves. upside_down_face

Changed the field to type. I'd like to keep using that name consistently in other discriminated unions that may come up in the project later on then.