V2 keysign message - DO NOT MERGE by johnnyluo · Pull Request #11 · vultisig/commondata

johnnyluo · 2024-08-05T03:21:30Z

This is the first design draft of KeysignMessage v2 , the goal of

V1 KeysignMessage is a bit confusing, and also pass some information that is not required.
for future proof, if third party integrate with Vultisign , they are working with some swaps that Vultisig doesn't aware of , make sure they can still fold their requirement into KeysignPayload support by Vultisig, thus Vultisig can co-sign it

enriquesouza

enum TransactionType {
TRANSACTION_TYPE_UNSPECIFIED = 0;
TRANSACTION_TYPE_VOTE = 1;
TRANSACTION_TYPE_PROPOSAL = 2;
}
TransactionType transaction_type = 7;

Maybe we can use this and add swap, send, and deposit to all chains, in case we need to add more options in the future.

johnnyluo · 2024-08-05T04:11:17Z

enum TransactionType { TRANSACTION_TYPE_UNSPECIFIED = 0; TRANSACTION_TYPE_VOTE = 1; TRANSACTION_TYPE_PROPOSAL = 2; } TransactionType transaction_type = 7;

Maybe we can use this and add swap, send, and deposit to all chains, in case we need to add more options in the future.

The problem is VOTE, PROPOSAL is only relevant for cosmos chain , not the others

yvebe · 2024-08-05T07:11:52Z

+  string to_address = 2;
+  string to_amount = 3;


maybe we should add coin, to_address, to_amount to a KeysignPayload, and reuse it from there? Seems like it repeated for each Transaction

Yup, quite a bunch of the information is repeated for each Transaction , if I move it to KeysignPayload , then it will end up more like v1

yeah, is it bad though for these two fields?

yvebe · 2024-08-05T07:13:05Z

+  uint64 account_number = 4;
+  uint64 sequence = 5;
+  uint64 fee = 6;
+  string is_deposit = 7; // probably don't need it , because when `to_address` is empty , it's a deposit , still keep it to make it specific


if to address can be empty, would be great to mark it as optional

All fields in protobuf is optional by default

agree, we can deprecate isDeposit

All fields in protobuf is optional by default

Code generators may not know that, so it's probably better to specify it, also that's self documenting

That's part of protobuf standard

yvebe · 2024-08-05T07:14:22Z

+  optional string memo = 10;
+}
+
+// SwapInfo is mostly for display purpose


is there any usecase for them when constructing transaction?

yes, for example , when we construct a swap transaction , swap itself will be put into transaction, for example , EVM chain swaps , at the end of the day , it is a standard transaction , or a smart contract call , which is also a standard transaction , but has some special data field

However , on the UI part , we often need to show user some additional information , like provider , fee , slippage , and final output amount etc. Those will go to swap_info.

swap_info is also optional , when it is empty , then it is just a standard send transaction

flypaper0 · 2024-08-05T11:37:58Z

+  int64 nonce = 6;
+  string gas_limit = 7;
+  optional string memo = 8;
+  optional vultisig.keysign.v1.Erc20ApprovePayload erc20_approve_payload = 9;


Something wrong here ig. ERC20 approve also regular evm tx. We need to cover case when we're need to generate two keysing messages. First - erc20 approve and second - erc20 transfer.

connected with comment: https://github.com/vultisig/commondata/pull/11/files#r1703993420

flypaper0 · 2024-08-05T11:44:37Z

+  // when swap_info is not empty , then the pair device shoudl treat it as a swap
+  // 1. Show swap verify screen
+  // 2. for EVM chain , need to construct a generic smart contract transaction
+  optional SwapInfo swap_info = 20;


let's make enum for display purposes that we can use for all UI cases?

enum Action { case transfer(TransferInfo) case swap(SwapInfo) case transferWithApproval(TransferWithApprovalInfo) case thorchainDeposit(ThorchainDepositInfo) }

We probably should save space for transactions, where this info is not needed. (e.g.: if all info for send tx available from already existing payload, we don't want to store more info)

exactly as what @yvebe mentioned , for Send , all the information is already in the transaction , only for swap , we need to have some additional information.
For those information already in the send transaction , we don't want to store it again here

flypaper0 · 2024-08-05T11:47:40Z

+  string target_amount = 5;
+}
+message KeysignPayload {
+  oneof transaction {


should this be an array of oneof transaction to support multiple signatures? Eg. erc20 transfer with approve or any other multiple tx signings

We can't do that , because if it is an array , it need to be the same type , but in our case , the transaction are all different types

johnnyluo added 2 commits August 5, 2024 13:16

V2 keysign message

2a2158e

fix errors

f71094d

enriquesouza reviewed Aug 5, 2024

View reviewed changes

enriquesouza approved these changes Aug 5, 2024

View reviewed changes

johnnyluo changed the title ~~V2 keysign message~~ V2 keysign message - DO NOT MERGE Aug 5, 2024

yvebe reviewed Aug 5, 2024

View reviewed changes

from_coin -> coin

7fc6b42

flypaper0 reviewed Aug 5, 2024

View reviewed changes

Conversation

johnnyluo commented Aug 5, 2024

Uh oh!

enriquesouza left a comment

Choose a reason for hiding this comment

Uh oh!

johnnyluo commented Aug 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yvebe Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yvebe Aug 5, 2024 •

edited

Loading