Add API for covenant genesis ID computation & output population#885
Add API for covenant genesis ID computation & output population#885smartgoo wants to merge 15 commits intokaspanet:tn12from
Conversation
IzioDev
left a comment
There was a problem hiding this comment.
Well needed addition for tn12!
consensus/wasm/src/hashing.rs
Outdated
| @@ -0,0 +1,35 @@ | |||
| use crate::error::Error; | |||
There was a problem hiding this comment.
I think it's fair to move the content to here: /consensus/client/src/hash.rs considering what it has at the moment
What do you think?
consensus/wasm/src/hashing.rs
Outdated
| /// | ||
| /// @category Consensus | ||
| #[wasm_bindgen(js_name = covenantId)] | ||
| pub fn covenant_id_js(genesis_outpoint: &TransactionOutpointT, auth_outputs: JsValue) -> Result<Hash> { |
There was a problem hiding this comment.
Why not using Vec<TransactionOutput> as the second argument here? I don't remember the exact behavior with Vec Js value parsing, but TransactionOutput does have a dedicated deserializer.
There was a problem hiding this comment.
@IzioDev great question, didn't think of trying to use Vec<_> here.
So each object in auth_outputs array needs to include an index in addition to the TransactionOutput instance/opbject. e.g.
[
{ index: 0, output: { value: 10000, scriptPublicKey: spk } },
...
]For that reason param type would need to be something like Vec<(u32, TransactionOutput)>.
But with that param type, compile error:
trait bound `(u32, kaspa_consensus_client::TransactionOutpoint): JsObject` is not satisfied
So my current thought is - not worth it to impl that for this use case. However, curious what you think. Also if you have any other thoughts on alternative approaches?
There was a problem hiding this comment.
@IzioDev Actually, even Vec<(u32, TransactionOutput)> is wrong.
I think it would be Vec<JsValue>, where JsValue looks like or would require some new struct e.g. AuthOutputs to use as Vec<AuthOutputs>. Where AuthOutputs probably looks like this:
struct AuthOutputs {
index: u32,
output: TransactionOutput
}There was a problem hiding this comment.
@IzioDev Ok I changed it to Vec<JsValue> in 2495111.
I feel this is a step in the right direction - it simplifies internals slightly and makes fn signature a little more explicit. Taking it a step further would be a custom struct a la the comment above.
However, I'm not sure how Vec<JsValue> plays with TypeScript. There are some ArrayT patterns used elsewhere for parameter types in WASM functions. Like PrivateKeyArrayT, PublicKeyArrayT, TransactionInputArrayAsArgT, TransactionOutputArrayAsArgT. Can explore this is you think it makes sense.
Sorry for yet another comment here.
consensus/wasm/src/lib.rs
Outdated
| pub mod hashing; | ||
| pub mod result; | ||
| mod utils; | ||
| pub use hashing::*; |
There was a problem hiding this comment.
Unrelated, but I'm sharing here in case :)
This has for effect to "spread" / "share" all of the public hashing members to the current crate, so within consensus/wasm you can do use crate::covenant_id_js; instead of use crate::hashing::covenant_id_js;
I'm not sure this was intended
There was a problem hiding this comment.
Ah yeah good call out. Used that approach since it was already used for pub use utils::*; a couple lines down. I intend to move it to other file per your other comment. So this will go away!
consensus/wasm/src/hashing.rs
Outdated
| /// | ||
| /// @category Consensus | ||
| #[wasm_bindgen(js_name = covenantId)] | ||
| pub fn covenant_id_js(genesis_outpoint: &TransactionOutpointT, auth_outputs: JsValue) -> Result<Hash> { |
There was a problem hiding this comment.
I'm not sure what's the general recommendation to enforce in term of naming, but many (if not all?) members of wallet/core/wasm define js (/wasm) functions as js_my_function (as a prefix)
There was a problem hiding this comment.
@IzioDev Looks like a mixed bag of js_ prefix and _js suffix throughout the codebase.
A quick (very incomplete) list to give a feel for it:
js_ prefix is used on functions js_sign_message, js_sign_verify_message, js_sign_transaction, everything in encryption.rs.
_js suffix is used on functions create_address_js, create_multisig_address_js, create_transaction_js, create_transactions_js, estimate_transactions_js, etc.
By quantity, it looks like a slight skew towards js_ prefix. So I have updated to use js prefix in a29c544. Thanks!
| /// WASM (TypeScript) type representing `ITransactionOutpoint | TransactionOutpoint` | ||
| /// @category Consensus |
There was a problem hiding this comment.
I'm not sure it's relevant to precise it's a wasm typescript type, end-users most likely don't care
There was a problem hiding this comment.
@IzioDev Admittedly I just copy/pasted this comment from other extern WASM blocks and changed out the actual type. This comment structure seems to be standard throughout WASM consensus client extern blocks - transaction input, transaction output, transaction for example use this same comment structure.
That said, happy to change it if you want!
|
@IzioDev TY for feedback (& the quick response)! Will get back to you on these in the next day or so. |
* use u32 for output indices (internal assumption anyhow) * add api parity tests
* add try from arrayt impl for genesis covenant group * switch error to already typed cast err * add error variant for invalid genesis covenant group array * switch error type * genesis covenant group wasm array try from and error update
|
Updated title & description to reflect expanded scope. Converted to draft status as this is WIP. Only a few remaining TODOs. |
* cctx populate_genesis_covenants fn * populate_genesis_covenants camel case js name * covenant getter/setter on client tx output * from client to core tx out with covenant
Summary:
This PR adds a standard process and helper function for covenant ID computation and population on genesis outputs:
kaspa-consensus-core'scovenant_id()function.Combined, these streamline the covenant genesis process, reducing lift for developers.
Note: this PR originally started as WASM SDK
covenant_id()function only. Scope changed to include functionality that further streamline the process.Why:
This codebase should provide a streamlined process and required building blocks for the covenant genesis process. Given the need to 1.) compute covenant ID for genesis; and .2) populate the covenant ID on genesis outputs.
This added functionality simplifies the process for Rust & WASM SDK developers.
Changes:
GenesisCovenantGroupandpopulate_genesis_covenants()fn to consensus core Transaction (by @michaelsutton)GenesisCovenantGroupto WASMjs_covenant_idinkaspa-consensus-clientcrate.ITransactionOutpointTS type inkaspa-consensus-clientcrate.populate_genesis_covenants()to consensus client TransactionWIP:
GenesisCovenantGroupWASM related code into consensus client, clean up errors, etc.)