Add ScriptPubKey model #370
Conversation
types/src/model/mod.rs
Outdated
| }, | ||
| }; | ||
|
|
||
| /// Models the data returned by Core for a script pubkey. |
There was a problem hiding this comment.
I believe 'script pubkey' is definitely wrong but I don't know if it should be scriptPubkey or scriptPubKey. See #371
There was a problem hiding this comment.
I have gone with scriptPubKey to match Core.
types/src/model/mod.rs
Outdated
| /// Script assembly. | ||
| pub asm: String, |
There was a problem hiding this comment.
We don't want this field. ScritpBuf has a method to get this. We could consider adding code during into_model that checks the string from Core against the string returned by rust-bitcoin but the we are probably just testing other crates ...
types/src/model/mod.rs
Outdated
| /// Script hex. | ||
| pub hex: ScriptBuf, |
There was a problem hiding this comment.
I'd put this as the first field and call it script, or script_pubkey, or inner. Anything but hex, which it no longer is.
There was a problem hiding this comment.
script_pubkey seems to be the most common name in the rest of the RPCs so I went with it.
types/src/model/mod.rs
Outdated
| /// The type, eg pubkeyhash. | ||
| pub type_: String, |
There was a problem hiding this comment.
Do we need this? We have predicates in rust-bitcoin eg is_p2pkh. I'm inclined to think drop it, its still there before one calls into_model if its needed. I'm not too fussed though.
There was a problem hiding this comment.
I don't think it is needed. As you say it is there before converting into the model type if needed.
types/src/v29/blockchain/error.rs
Outdated
| Script(ref e) => write_err!(f, "conversion of the script `hex` field failed"; e), | ||
| Address(ref e) => write_err!(f, "conversion of the `address` field failed"; e), | ||
| ActivityEntry(ref e) => write_err!(f, "conversion of an activity entry failed"; e), | ||
| PrevoutSPK(ref e) => write_err!(f, "conversion of the `prevout_spk` field failed"; e), |
There was a problem hiding this comment.
Rust convention for accronyms is just to capitalize the first character. So PrevoutsSpk
jamillambert
force-pushed
the
0930-scriptpubkey
branch
from
f43818e to
0c85737
Compare
types/src/model/mod.rs
Outdated
| /// This is used by methods in the blockchain section and in the raw transaction section (i.e raw | ||
| /// transaction and psbt methods). The shape changed in Core v22 but the new shape is fully | ||
| /// backwards compatible so we only provide it not a v0.17 specific type. |
There was a problem hiding this comment.
| /// This is used by methods in the blockchain section and in the raw transaction section (i.e raw | |
| /// transaction and psbt methods). The shape changed in Core v22 but the new shape is fully | |
| /// backwards compatible so we only provide it not a v0.17 specific type. | |
| /// This is used by methods in the blockchain section and in the raw transaction section (i.e raw | |
| /// transaction and psbt methods). |
That other bit is not relevant here in model, right?
There was a problem hiding this comment.
Yeah it's not relevant here. I removed it.
types/src/model/mod.rs
Outdated
| /// Only returned before in versions prior to 22 or for version 22 onwards if | ||
| /// config option `-deprecatedrpc=addresses` is passed. |
There was a problem hiding this comment.
| /// Only returned before in versions prior to 22 or for version 22 onwards if | |
| /// config option `-deprecatedrpc=addresses` is passed. | |
| /// Only returned in versions prior to 22 or for version 22 onwards if | |
| /// config option `-deprecatedrpc=addresses` is passed. |
There was a problem hiding this comment.
There are 4 instances of this typo in the codebase:
gg 'returned before in'
lib.rs:196: /// Only returned before in versions prior to 22 or for version 22 onwards if
lib.rs:207: /// Only returned before in versions prior to 22 or for version 22 onwards if
model/mod.rs:85: /// Only returned before in versions prior to 22 or for version 22 onwards if
model/mod.rs:92: /// Only returned before in versions prior to 22 or for version 22 onwards if
ScriptPubKey has fields that can be modeled with rust-bitcoin types. Add a model, an into_model function and error.
A new model with strongly typed fields for ScriptPubKey has been added. Update GetDescriptorActivity model to use it. Add the required error variants and into_model function.
jamillambert
force-pushed
the
0930-scriptpubkey
branch
from
0c85737 to
478a53c
Compare
2 participants
ScriptPubKeyhas fields that can be modelled with rust-bitcoin types. And says in it's own rustdocThe `mtype::ScriptPubkey` mirrors this design, but there is none.Two RPCs use this type,
getdescriptoractivityandgettxout. TheGetTxOutmodel extracts individual fields from theScriptPubKeyto create aTxOutandAddress, and does not model aScriptPubKeytype.ScriptPubKeymodel, error and into function.GetDescriptorActivity.