Fix (de)serialization of transaction for adding subnet validators #180
Conversation
| @@ -342,6 +342,7 @@ fn test_sort_output_owners() { | |||
| /// ref. <https://pkg.go.dev/github.com/ava-labs/avalanchego/vms/secp256k1fx#Input> | |||
| #[derive(Debug, Serialize, Deserialize, Eq, Clone, Default)] | |||
| pub struct Input { | |||
| #[serde(rename = "signatureIndices")] | |||
There was a problem hiding this comment.
Nit:
Generally, it's probably better to do $[serde(rename_all = "camelCase")] so that you don't have to add this to every new field.
There was a problem hiding this comment.
Sorry, forgot to mention that you would also need to rename the field to signature_indices.
There was a problem hiding this comment.
If you don't want to do this, I will still accept the PR
There was a problem hiding this comment.
Nit: Generally, it's probably better to do
$[serde(rename_all = "camelCase")]so that you don't have to add this to every new field.
Yeah, I've been considering that, but it doesn't quite work for some fields like node_id. Still, may be better to just add it.
| pub subnet_auth: key::secp256k1::txs::Input, | ||
|
|
||
| /// To be updated after signing. | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] |
There was a problem hiding this comment.
Is the skip_serializing_if actually necessary? Will an empty vector break something?
| /// ref. <https://pkg.go.dev/github.com/ava-labs/avalanchego/vms/platformvm/txs#Tx.Sign> | ||
| /// ref. <https://pkg.go.dev/github.com/ava-labs/avalanchego/utils/crypto#PrivateKeyED25519.SignHash> | ||
| pub async fn sign<T: key::secp256k1::SignOnly>(&mut self, signers: Vec<Vec<T>>) -> Result<()> { | ||
| pub fn pack(&self) -> Result<packer::Packer> { |
There was a problem hiding this comment.
Doesn't seem like there's any reason to change the public API here
| pub fn pack(&self) -> Result<packer::Packer> { | |
| fn pack(&self) -> Result<packer::Packer> { |
| @@ -405,3 +421,153 @@ fn test_add_subnet_validator_tx_serialization_with_one_signer() { | |||
| &tx_bytes_with_signatures | |||
| )); | |||
| } | |||
|
|
|||
| /// RUST_LOG=debug cargo test --package avalanche-types --lib -- platformvm::txs::add_subnet_validator::test_json_deserialize --exact --show-output | |||
There was a problem hiding this comment.
I know you're following a pattern, but I'm really not a fan of this. There are clear docs for how to run tests, there's no point in duplicating them and having to change this if we move any files/modules around.
| /// RUST_LOG=debug cargo test --package avalanche-types --lib -- platformvm::txs::add_subnet_validator::test_json_deserialize --exact --show-output | |
| /// RUST_LOG=debug cargo test --package avalanche-types --lib -- platformvm::txs::add_subnet_validator::test_json_deserialize --exact --show-output |
| "amount": 19999999899000000 as u64, | ||
| "locktime": 0, | ||
| "threshold": 1 | ||
| } | ||
| } | ||
| ], | ||
| "inputs": [ | ||
| { | ||
| "txID": "CjisYCwF4j7zSyC25MWR21e5xxzJgwdaLEuf7oBXSGpe3oaej", | ||
| "outputIndex": 0, | ||
| "assetID": "28VTWcsTZ55draGkmjdcS9CFFv4zC3PbvVjkyoqxzNC7Y5msRP", | ||
| "fxID": "spdxUxVJQbX85MGxMHbKw1sHxMnSqJ3QBzDyDYEP3h6TLuxqQ", | ||
| "input": { "amount": 19999999900000000 as u64, "signatureIndices": [0] } |
There was a problem hiding this comment.
| "amount": 19999999899000000 as u64, | |
| "locktime": 0, | |
| "threshold": 1 | |
| } | |
| } | |
| ], | |
| "inputs": [ | |
| { | |
| "txID": "CjisYCwF4j7zSyC25MWR21e5xxzJgwdaLEuf7oBXSGpe3oaej", | |
| "outputIndex": 0, | |
| "assetID": "28VTWcsTZ55draGkmjdcS9CFFv4zC3PbvVjkyoqxzNC7Y5msRP", | |
| "fxID": "spdxUxVJQbX85MGxMHbKw1sHxMnSqJ3QBzDyDYEP3h6TLuxqQ", | |
| "input": { "amount": 19999999900000000 as u64, "signatureIndices": [0] } | |
| "amount": 19999999899000000_u64, | |
| "locktime": 0, | |
| "threshold": 1 | |
| } | |
| } | |
| ], | |
| "inputs": [ | |
| { | |
| "txID": "CjisYCwF4j7zSyC25MWR21e5xxzJgwdaLEuf7oBXSGpe3oaej", | |
| "outputIndex": 0, | |
| "assetID": "28VTWcsTZ55draGkmjdcS9CFFv4zC3PbvVjkyoqxzNC7Y5msRP", | |
| "fxID": "spdxUxVJQbX85MGxMHbKw1sHxMnSqJ3QBzDyDYEP3h6TLuxqQ", | |
| "input": { "amount": 19999999900000000_64, "signatureIndices": [0] } |
There was a problem hiding this comment.
Sorry, I was running into issues with _u64, because it somehow still ended up being treated as an i32. With a u64 suffix, it works now.
| @@ -21,6 +21,7 @@ pub struct Tx { | |||
| pub shares: u32, | |||
|
|
|||
| /// To be updated after signing. | |||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | |||
There was a problem hiding this comment.
Same question: does it matter if we serialize the empty vector?
Co-authored-by: Richard Pringle <[email protected]> Signed-off-by: Andres Noetzli <[email protected]>
|
@richardpringle Thank you for the review! I believe I've addressed all of your comments. |
|
Not sure what's going on with the tests here. Are you running |
Sorry, there were some places where I didn't update the names. Should be fixed now. |
No description provided.