-
Notifications
You must be signed in to change notification settings - Fork 1
Symbol - Transactions #9
Comments
When signing and cosign transactions, we should try to abstract the signing process. It can happen that the account doesn't know a private key (like Ledger and Trezor). In the old SDK, we assumed that all the accounts have the private key, and then the apps that use Ledger need to copy and paste core code for merging signatures and cosignatures (payload processing). You can see the duplicated code: https://github.com/symbol/desktop-wallet/blob/main/src/core/utils/Ledger.ts#L157 Something inline of https://github.com/symbol/symbol-cli/blob/ledger_support/src/services/signing.service.ts#L44 probably. Basically use a SigningAccount/Signer interface (instead of just the Account with private key) that knows how to sign/cosign a transaction. The private key account one would be the out-of-the-box signer. |
I had a chat with @rg911 about how to represent the symbol transaction model in the SDK. We evaluated 2 alternatives: 1 - Create the user-friendly transaction hierarchy as we have in the legacy SDK. Both have pros and cons. Transaction/State wrappers.
Catbuffer builders as-is
We agreed to change the approach this time, the user would use the catbuffer builders to handle transactions (Option 2). For this, I think we would need
This idea can be applied to other states like accounts/metadata for merkle proofs. What about NIS1? Is a NIS1 catbuffer and generator possible? If we add that to the catbuffer schemas, we may get all the builders for free.... Catbuffer schemas could be both symbol+nis1 schemas like we are unifying the SDKs. I will be pushing a few PRs. |
I would prefer using catbuffer builder directly, however with slight mods:
|
PR incoming :)
Unsure how that can look (factory of a builder?), the examples could give us an indication how useful they are. Ideally, any transaction related code should be generated so it's free for all the transactions. If the factory works, we can generate it from the schema. |
No description provided.
The text was updated successfully, but these errors were encountered: