Improve ABI types in wasmv1 #17

bilboquet · 2023-06-19T07:38:21Z

No description provided.

aoudiamoncef

LGTM good job 👏

Just add missing comments on messages and field and It's ready to ship

proto/abis/massa/abi/v1/abi.proto

damip · 2023-06-19T08:13:30Z

* use https://protobuf.dev/reference/csharp/api-docs/class/google/protobuf/well-known-types/empty instead of our own Empty

* use bytes in GenerateEvent but have the default SDK take strings and convert them to UTF8 when calling the ABI

* SubNativeAmountsRequest
	use "left" and "right" and explain that we compute "left - right, fail if right > left" instead of "minuend/subtrahend"

* NativeAmountToBytesRequest
	technically the SC can already do to/from byte conversion !
        I would vote for removing those to/from byte conversions and just use the builtin SC-based deserializer together with the Check* ABIs to verify the integrity of the data

* remove LogRequest: we are deprecating log printing in favor of Events

bilboquet · 2023-06-19T08:22:27Z

LGTM good job clap

Just add missing comments on messages and field and It's ready to ship

Yes, it's still draft, because up to now I was still concerned by the structure of the .proto. (it was hardly extensible and not that easy to manipulate in Rust)
I think it's now ok, hence the PR :)
And so I can:
[] add comments
[] verify int vs fixed

Signed-off-by: Jean-François Morcillo <[email protected]>

aoudiamoncef · 2023-06-19T09:19:05Z

IMHO, using well known type Empty could be a breaking change in the future.

In the first version private API, I tried to use a xxxRequest xxxResponse even it's an empty. If we would like to change input/output of the method = no breaking change

bilboquet · 2023-06-19T09:47:21Z

IMHO, using well known type Empty could be a breaking change in the future.

In the first version private API, I tried to use a xxxRequest xxxResponse even it's an empty. If we would like to change input/output of the method = no breaking change

Hum… You're suggesting to not use it directly but to encapsulate it right?
like

message FakeAbiResult {
  Wellknow::Empty res;
}

(you are not suggesting to use our own Empty definition, right)

If in both case I'm correct, we are ok. If I'm wrong somewhere I need you to elaborate on your point because I don't understand it.

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet · 2023-06-19T11:51:40Z

LGTM good job clap
Just add missing comments on messages and field and It's ready to ship

Yes, it's still draft, because up to now I was still concerned by the structure of the .proto. (it was hardly extensible and not that easy to manipulate in Rust) I think it's now ok, hence the PR :) And so I can:

[X] add comments
[X] verify int vs fixed

bilboquet · 2023-06-19T11:53:23Z

proto/abis/massa/abi/v1/abi.proto

 }

 // LocalCallResponse
 message LocalCallResponse {
  // Return_data is the return value of the function
-  bytes return_data = 1;
+  bytes data = 1;
 }

 // GenerateEventRequest


use bytes in GenerateEvent but have the default SDK take strings and convert them to UTF8 when calling the ABI

@damip
just for information, the current wip implementation of this ABI in the AS SDK already does the conversion from UTF16 (user side) <--> UTF8 (abi side)
basically it's just adding a to_vec

Basically in the SC SDK we would take a utf16 string as argument and convert it to utf8 then to bytes before sending it to the abi

That's what I was trying to say :)

bilboquet · 2023-06-19T12:07:01Z

use https://protobuf.dev/reference/csharp/api-docs/class/google/protobuf/well-known-types/empty instead of our own Empty

Own type removed, will use the one from google when needed (if the discussion in the other comment is correct)
[X] done

use bytes in GenerateEvent but have the default SDK take strings and convert them to UTF8 when calling the ABI

added a dedicated comment to track this point (so consider this one closed)
[X] done

SubNativeAmountsRequest use "left" and "right" and explain that we compute "left - right, fail if right
left" instead of "minuend/subtrahend"
[X] done

NativeAmountToBytesRequest technically the SC can already do to/from byte conversion ! I would vote for removing those to/from byte conversions and just use the builtin SC-based deserializer together with the Check * ABIs to verify the integrity of the data

I took those 2 from the original discussion without much thinking... I fully agree that if the SC can do it by itself we can remove them.
[X] done

remove LogRequest: we are deprecating log printing in favor of Events`

moved in a separate branch as this is very useful when writing the sdk, but for sure it should not be in the final ABI
[X] done

proto/abis/massa/abi/v1/abi.proto

bilboquet · 2023-06-19T12:16:45Z

proto/abis/massa/abi/v1/abi.proto

-  // Address is a string representation of the address
-  Address address = 1;
+  // Address of the just created smart contract
+  massa.model.v1.NativeAddress sc_address = 1;
 }

 // CallSC
 message CallRequest {


DON'T Transform to the Response<Result,Error> form

bilboquet · 2023-06-19T12:17:02Z

proto/abis/massa/abi/v1/abi.proto

 }

 // CallResponse
 message CallResponse {
  // Return_data is the return value of the function
-  bytes return_data = 1;
+  bytes data = 1;
 }

 // LocalCall
 message LocalCallRequest {


DON'T Transform to the Response<Result,Error> form

bilboquet · 2023-06-19T12:18:24Z

proto/abis/massa/abi/v1/abi.proto

@@ -82,22 +76,319 @@ message GenerateEventRequest {

 // TransferCoins
 message TransferCoinsRequest {


Transform to the Response<Result,Error> form (later)

bilboquet · 2023-06-19T12:18:35Z

proto/abis/massa/abi/v1/abi.proto

+  // The address of the recipient
+  massa.model.v1.NativeAddress target_address = 1;
+  // The amount of coins to transfer
+  massa.model.v1.NativeAmount amount_to_transfer = 2;
 }

 // FunctionExists
 message FunctionExistsRequest {


Transform to the Response<Result,Error> form (later)

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet · 2023-06-19T13:14:38Z

proto/abis/massa/abi/v1/abi.proto

@@ -392,3 +399,107 @@ message DivRemNativeAmountResult {
  // Remainder of amount and divisor
  massa.model.v1.NativeAmount remainder = 2;
 }
+
+// Comparison result
+message ComparisonResult {


@aoudiamoncef
Do you think I should put this one in commons?
I feel like it should be

Yes I think so too, a bit like https://doc.rust-lang.org/std/cmp/enum.Ordering.html

let's agree on the path before I move it
I propose
proto/commons/massa/shared/v1/comparison.proto

it follow the scheme of others
proto/abis/massa/abi/v1/abi.proto
proto/abis/massa/abi/v1/abi.proto
proto/commons/massa/model/v1/address.proto...

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet · 2023-06-19T14:05:56Z

proto/abis/massa/abi/v1/abi.proto

+// keccak256 hash result
+message Keccak256Result {
+  // Hash of data
+  bytes hash = 1;


@damip
What about returning a NativeHash?
(same would apply EVM verify, why not passing sig as a NativeSig?)

For native Massa stuff we should create types.
For foreign objects (eg. ETH hashes) we can stay in raw bytes

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet · 2023-06-19T17:16:18Z

proto/abis/massa/abi/v1/abi.proto

+}
+
+// EVM signature verification request
+message VerifyEvmSigRequest {


For every Verify*SigRequest
put Sig as the first parameter

Signed-off-by: Jean-François Morcillo <[email protected]>

* WIP improve ABI types Signed-off-by: Jean-François Morcillo <[email protected]> * Factorize Result messages Signed-off-by: Jean-François Morcillo <[email protected]> * Response to some comments in #17 Signed-off-by: Jean-François Morcillo <[email protected]> * Add Time related operations Signed-off-by: Jean-François Morcillo <[email protected]> * Add CompareNative[Address|PubKey|Sig] Signed-off-by: Jean-François Morcillo <[email protected]> * Add VerifyNativeSigRequest and fixed -> int Signed-off-by: Jean-François Morcillo <[email protected]> * Add CompareNativeAmount and Keccak256 hashing Signed-off-by: Jean-François Morcillo <[email protected]> * Add VerifyEvmSigResult VerifyEvmSigResult VerifyEvmSigResult Signed-off-by: Jean-François Morcillo <[email protected]> * Add TransferCoinsResult GenerateEventResult Signed-off-by: Jean-François Morcillo <[email protected]> * For Verify*Request sig is the first argument Signed-off-by: Jean-François Morcillo <[email protected]> * Add CreateSCResult Signed-off-by: Jean-François Morcillo <[email protected]> * Add FunctionExistsResult Signed-off-by: Jean-François Morcillo <[email protected]> * Add all but 1 request/result for legacy abi Signed-off-by: Jean-François Morcillo <[email protected]> * Various .proto improvements (#19) * Unify current_thread and current_period * Removed Non 'For' ABIs * Updated numbers in ABI Results * Removed logs / trace / prints * Forgot to remove some duplicated definitions * Apply modification from issue #246 see massalabs/massa-sc-runtime#246 (comment) for details GenerateEvent takes bytes Use the optional keyword when appropriate Use StringValue for optional string Signed-off-by: Jean-François Morcillo <[email protected]> * Added prefix to GetOpKeysRequest (#24) * Use google.protobuf.xIntxxValue for int values to discriminate 0 form none Signed-off-by: Jean-François Morcillo <[email protected]> * unsafe random and send async message types * add to oneof * add local_execution_response to oneof * doc * regen doc * Revert Use google.protobuf.xIntxxValue for int values to discriminate 0 form none Signed-off-by: Jean-François Morcillo <[email protected]> * Fix PR comment Signed-off-by: Jean-François Morcillo <[email protected]> * Remove useless CheckBlake3HashRequest Signed-off-by: Jean-François Morcillo <[email protected]> * Remove useless CompareSigRequest Signed-off-by: Jean-François Morcillo <[email protected]> * GetOriginOperationId request & result + doc * Reference * Fix DateNowResult, remove useless ProcessExitResult Signed-off-by: Jean-François Morcillo <[email protected]> * Remove unused VerifyBlsSingleSigRequest, VerifyBlsMultiSigRequest, SeedRequest, DateNowRequest, ProcessExitRequest Signed-off-by: Jean-François Morcillo <[email protected]> * Rename Blake3HashRequest to HashBlake3Request for consistency Signed-off-by: Jean-François Morcillo <[email protected]> * Better names Signed-off-by: Jean-François Morcillo <[email protected]> * Evm requests & results --------- Signed-off-by: Jean-François Morcillo <[email protected]> Co-authored-by: Jean-François Morcillo <[email protected]> Co-authored-by: Leo-Besancon <[email protected]> Co-authored-by: Thomas Plisson <[email protected]> Co-authored-by: Eitu33 <[email protected]>

bilboquet requested review from damip, aoudiamoncef, Leo-Besancon and Eitu33 June 19, 2023 07:39

bilboquet force-pushed the bilboquet/Improve_ABI_types_in_wasmv1_WIP branch from 02e2484 to 6f84ac0 Compare June 19, 2023 07:46

bilboquet mentioned this pull request Jun 19, 2023

Improve ABI types in wasmv1 massalabs/massa-sc-runtime#246

Closed

aoudiamoncef approved these changes Jun 19, 2023

View reviewed changes

Jean-François Morcillo added 2 commits June 19, 2023 10:49

WIP improve ABI types

2c0a935

Signed-off-by: Jean-François Morcillo <[email protected]>

Factorize Result messages

c62e0a8

Signed-off-by: Jean-François Morcillo <[email protected]>

Response to some comments in #17

49ee9a7

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet force-pushed the bilboquet/Improve_ABI_types_in_wasmv1_WIP branch from 6f84ac0 to 49ee9a7 Compare June 19, 2023 11:49

bilboquet commented Jun 19, 2023

View reviewed changes

proto/abis/massa/abi/v1/abi.proto Outdated Show resolved Hide resolved

bilboquet commented Jun 19, 2023

View reviewed changes

Add Time related operations

e8d96c8

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet commented Jun 19, 2023

View reviewed changes

Jean-François Morcillo added 3 commits June 19, 2023 15:40

Add CompareNative[Address|PubKey|Sig]

de42d76

Signed-off-by: Jean-François Morcillo <[email protected]>

Add VerifyNativeSigRequest and fixed -> int

f643139

Signed-off-by: Jean-François Morcillo <[email protected]>

Add CompareNativeAmount and Keccak256 hashing

14b13e3

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet commented Jun 19, 2023

View reviewed changes

Add VerifyEvmSigResult VerifyEvmSigResult VerifyEvmSigResult

d9d4c63

Signed-off-by: Jean-François Morcillo <[email protected]>

Add TransferCoinsResult GenerateEventResult

0d728d5

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet commented Jun 19, 2023

View reviewed changes

Jean-François Morcillo added 3 commits June 20, 2023 10:55

For Verify*Request sig is the first argument

d44c932

Signed-off-by: Jean-François Morcillo <[email protected]>

Add CreateSCResult

b118730

Signed-off-by: Jean-François Morcillo <[email protected]>

Add FunctionExistsResult

0f9e0ed

Signed-off-by: Jean-François Morcillo <[email protected]>

bilboquet closed this Jul 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ABI types in wasmv1 #17

Improve ABI types in wasmv1 #17

bilboquet commented Jun 19, 2023

aoudiamoncef left a comment

damip commented Jun 19, 2023 •

edited

Loading

bilboquet commented Jun 19, 2023

aoudiamoncef commented Jun 19, 2023

bilboquet commented Jun 19, 2023

bilboquet commented Jun 19, 2023

bilboquet Jun 19, 2023

bilboquet Jun 19, 2023

damip Jun 19, 2023

bilboquet Jun 19, 2023

bilboquet commented Jun 19, 2023 •

edited

Loading

bilboquet Jun 19, 2023

bilboquet Jun 19, 2023

bilboquet Jun 19, 2023

bilboquet Jun 19, 2023

bilboquet Jun 19, 2023

damip Jun 19, 2023

bilboquet Jun 19, 2023

bilboquet Jun 19, 2023 •

edited

Loading

damip Jun 26, 2023

bilboquet Jun 19, 2023

		@@ -82,22 +76,319 @@ message GenerateEventRequest {

		// TransferCoins
		message TransferCoinsRequest {

Improve ABI types in wasmv1 #17

Improve ABI types in wasmv1 #17

Conversation

bilboquet commented Jun 19, 2023

aoudiamoncef left a comment

Choose a reason for hiding this comment

damip commented Jun 19, 2023 • edited Loading

bilboquet commented Jun 19, 2023

aoudiamoncef commented Jun 19, 2023

bilboquet commented Jun 19, 2023

bilboquet commented Jun 19, 2023

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

bilboquet commented Jun 19, 2023 • edited Loading

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

Choose a reason for hiding this comment

bilboquet Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damip commented Jun 19, 2023 •

edited

Loading

bilboquet commented Jun 19, 2023 •

edited

Loading

bilboquet Jun 19, 2023 •

edited

Loading