💡 [REQUEST] - Add comment support to `clauseBuilder`

[ifavo]

Summary

The clauseBuilder generates raw clauses without comments. Vechain Wallets support the display of clause related comments that improve user experience greatly.

Adding support for comments during clause building can help improve developer experience, because built clause objects need to be manipulated in a separate step. Also the knowledge about comments will be more visible, if the builder will support comments.

Basic Example

// current handling
const transferClause = clauseBuilder.transferVET('0x0000000000000000000000000000456e65726779', 1),
transferClause.comment = 'Transfer VET to VTHO contract'

await connex.vendor.sign('tx', [transferClause]).request()

// two suggestions on how to add options
clauseBuilder.transferVET('0x0000000000000000000000000000456e65726779', 1).comment('Transfer VET to VTHO contract')
clauseBuilder.transferVET('0x0000000000000000000000000000456e65726779', 1, {comment: 'Transfer VET to VTHO contract'})

// this can make transaction building simpler
await connex.vendor.sign('tx', [ 
  clauseBuilder.transferVET('0x0000000000000000000000000000456e65726779', 1, {comment: 'Transfer VET to VTHO contract'}),
  clauseBuilder.transferToken('0x0000000000000000000000000000456e65726779', '0x0000000000000000000000000000456e65726779', 1, {comment: 'Transfer VTHO to VTHO contract'})
]).request()

[darrenvechain]

Can we also add ABI support, so we're fully compatible with current vechain signers?

I guess the contract can set the ABI, but only if the client requests it or something?

https://github.com/vechain/connex/blob/master/packages/types/vendor.d.ts#L64

[ifavo]

Can we also add ABI support, so we're fully compatible with current vechain signers?

I was somehow expecting this to happen already automatically in the background for the clauseBuilder functions. When transferToken is executed, that it will automatically pass on the ABI information for decoding support. If this is not the case, I think the clauseBuilder should fill in the data automatically without any action required from the developer invoking it.

[ifavo]

@darrenvechain This looks like a good spot to add it and having it enabled for all calls by default:

https://github.com/vechain/vechain-sdk-js/blob/b27bb34a6b5e6af44daa8362c79c07e06d6cd091/packages/core/src/clause/clause.ts#L46-L57

Is it possible that the TransactionClause is missing the TxMessage you've linked?

[darrenvechain]

Can we also add ABI support, so we're fully compatible with current vechain signers?

I was somehow expecting this to happen already automatically in the background for the clauseBuilder functions. When transferToken is executed, that it will automatically pass on the ABI information for decoding support. If this is not the case, I think the clauseBuilder should fill in the data automatically without any action required from the developer invoking it.

I wouldn't automatically add it, because if you're estimating gas you likely end up with a bad request in the response, eg.:

curl --request POST \
  --url 'https://mainnet.vechain.org/accounts/*' \
  --header 'Accept: application/json, text/plain' \
  --header 'Content-Type: application/json' \
  --data '{
    "gas": 50000,
    "gasPrice": "1000000000000000",
    "caller": "0x6d95e6dca01d109882fe1726a2fb9865fa41e7aa",
    "provedWork": "1000",
    "gasPayer": "0xd3ae78222beadb038203be21ed5ce7c9b1bff602",
    "expiration": 1000,
    "blockRef": "0x00000000851caf3c",
    "clauses": [
        {
            "to": "0x0000000000000000000000000000456E65726779",
            "abi": {},
            "value": "0x0",
            "data": "0xa9059cbb0000000000000000000000000f872421dc479f3c11edd89512731814d0598db50000000000000000000000000000000000000000000000013f306a2409fc0000"
        }
    ]
}'

[darrenvechain]

@darrenvechain This looks like a good spot to add it and having it enabled for all calls by default:

https://github.com/vechain/vechain-sdk-js/blob/b27bb34a6b5e6af44daa8362c79c07e06d6cd091/packages/core/src/clause/clause.ts#L46-L57

Is it possible that the TransactionClause is missing the TxMessage you've linked?

TransactionClause is just to, value and data, I think we need some form of ExtendedClause is needed when interacting with wallets

[darrenvechain]

IMO acould approach could be some optional options when building clauses:

type ClauseOptions = {
   ...existing,
   comment?: string
   includeAbi?: boolean
}