Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(proto)!: update SignedTransaction to contain Any for transaction #1044

Merged
merged 10 commits into from
May 10, 2024
Merged

refactor(proto)!: update SignedTransaction to contain Any for transaction #1044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
655dbde
update SignedTransaction to contain Any for transaction
noot May 3, 2024
deb23aa
Merge branch 'main' of github.com:astriaorg/astria into noot/tx-protobuf
noot May 3, 2024
d8b1906
clippy
noot May 3, 2024
fa08970
clippy
noot May 3, 2024
ddd02ee
Merge branch 'main' of github.com:astriaorg/astria into noot/tx-protobuf
noot May 8, 2024
41e8aca
Merge branch 'main' of github.com:astriaorg/astria into noot/tx-protobuf
noot May 8, 2024
520775b
address comments
noot May 8, 2024
bdb57e5
update snapshot test
noot May 8, 2024
ee4a397
Merge branch 'main' of github.com:astriaorg/astria into noot/tx-protobuf
noot May 9, 2024
f6d39a2
add transaction_bytes to SignedTransaction
noot May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/astria-core/src/generated/astria.protocol.transactions.v1alpha1.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

113 changes: 105 additions & 8 deletions crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ use ed25519_consensus::{
SigningKey,
VerificationKey,
};
use prost::Message as _;
use prost::{
Message as _,
Name as _,
};

use super::raw;

Expand Down Expand Up @@ -68,6 +71,7 @@ pub struct SignedTransaction {
signature: Signature,
verification_key: VerificationKey,
transaction: UnsignedTransaction,
transaction_bytes: bytes::Bytes,
}

impl SignedTransaction {
Expand All @@ -90,12 +94,16 @@ impl SignedTransaction {
let Self {
signature,
verification_key,
transaction,
transaction_bytes,
..
} = self;
raw::SignedTransaction {
signature: signature.to_bytes().to_vec(),
public_key: verification_key.to_bytes().to_vec(),
transaction: Some(transaction.into_raw()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case for calling into_raw or to_raw on a SignedTransaction? Since we don't persist the serialized bytes in try_from_raw we lose the determinism of the what is returned here. The signature may not match the transaction bytes on a roundtrip. If you deserialized from raw, reserializing into is not safe.

My thinking is we can either remove this if there isn't a use for them, or we should maintain a copy of the bytes of the transaction when creating SignedTransaction and then build and return the any object here. I know not as pretty, and in some usecases it would be fine (ie I just constructed a new transaction and signed it), but the loops wouldn't be safe and avoiding them ever happening would require we add some data saying like "derived from raw" and then not allow reserializing (assuming we aren't perfect at code review).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i updated this to keep the transaction_bytes in SignedTransaction so converting to raw is now guaranteed to be the same

transaction: Some(pbjson_types::Any {
type_url: raw::UnsignedTransaction::type_url(),
value: transaction_bytes,
}),
}
}

Expand All @@ -104,12 +112,16 @@ impl SignedTransaction {
let Self {
signature,
verification_key,
transaction,
transaction_bytes,
..
} = self;
raw::SignedTransaction {
signature: signature.to_bytes().to_vec(),
public_key: verification_key.to_bytes().to_vec(),
transaction: Some(transaction.to_raw()),
transaction: Some(pbjson_types::Any {
type_url: raw::UnsignedTransaction::type_url(),
value: transaction_bytes.clone(),
}),
}
}

Expand All @@ -135,16 +147,17 @@ impl SignedTransaction {
let Some(transaction) = transaction else {
return Err(SignedTransactionError::unset_transaction());
};
let bytes = transaction.encode_to_vec();
let bytes = transaction.value.clone();
verification_key
.verify(&signature, &bytes)
.map_err(SignedTransactionError::verification)?;
let transaction = UnsignedTransaction::try_from_raw(transaction)
let transaction = UnsignedTransaction::try_from_any(transaction)
.map_err(SignedTransactionError::transaction)?;
Ok(Self {
signature,
verification_key,
transaction,
transaction_bytes: bytes,
})
}

Expand All @@ -155,6 +168,7 @@ impl SignedTransaction {
signature,
verification_key,
transaction,
..
} = self;
SignedTransactionParts {
signature,
Expand Down Expand Up @@ -211,6 +225,7 @@ impl UnsignedTransaction {
signature,
verification_key,
transaction: self,
transaction_bytes: bytes.into(),
}
}

Expand All @@ -226,6 +241,15 @@ impl UnsignedTransaction {
}
}

#[must_use]
pub fn into_any(self) -> pbjson_types::Any {
let raw = self.into_raw();
pbjson_types::Any {
type_url: raw::UnsignedTransaction::type_url(),
value: raw.encode_to_vec().into(),
}
}

pub fn to_raw(&self) -> raw::UnsignedTransaction {
let Self {
actions,
Expand All @@ -239,6 +263,11 @@ impl UnsignedTransaction {
}
}

#[must_use]
pub fn to_any(&self) -> pbjson_types::Any {
self.clone().into_any()
}

/// Attempt to convert from a raw, unchecked protobuf [`raw::UnsignedTransaction`].
///
/// # Errors
Expand All @@ -265,6 +294,22 @@ impl UnsignedTransaction {
params,
})
}

/// Attempt to convert from a protobuf [`pbjson_types::Any`].
///
/// # Errors
///
/// - if the type URL is not the expected type URL
/// - if the bytes in the [`Any`] do not decode to an [`UnsignedTransaction`]
pub fn try_from_any(any: pbjson_types::Any) -> Result<Self, UnsignedTransactionError> {
if any.type_url != raw::UnsignedTransaction::type_url() {
return Err(UnsignedTransactionError::invalid_type_url(any.type_url));
}

let raw = raw::UnsignedTransaction::decode(any.value)
.map_err(UnsignedTransactionError::decode_any)?;
Self::try_from_raw(raw)
}
}

#[derive(Debug, thiserror::Error)]
Expand All @@ -279,6 +324,16 @@ impl UnsignedTransactionError {
fn unset_params() -> Self {
Self(UnsignedTransactionErrorKind::UnsetParams())
}

fn invalid_type_url(got: String) -> Self {
Self(UnsignedTransactionErrorKind::InvalidTypeUrl {
got,
})
}

fn decode_any(inner: prost::DecodeError) -> Self {
Self(UnsignedTransactionErrorKind::DecodeAny(inner))
}
}

#[derive(Debug, thiserror::Error)]
Expand All @@ -287,6 +342,17 @@ enum UnsignedTransactionErrorKind {
Action(#[source] action::ActionError),
#[error("`params` field is unset")]
UnsetParams(),
#[error(
"encountered invalid type URL when converting from `google.protobuf.Any`; got `{got}`, \
expected `{}`",
raw::UnsignedTransaction::type_url()
)]
InvalidTypeUrl { got: String },
#[error(
"failed to decode `google.protobuf.Any` to `{}`",
raw::UnsignedTransaction::type_url()
)]
DecodeAny(#[source] prost::DecodeError),
}

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -367,9 +433,40 @@ mod test {
let tx = SignedTransaction {
signature,
verification_key,
transaction: unsigned,
transaction: unsigned.clone(),
transaction_bytes: unsigned.to_raw().encode_to_vec().into(),
};

insta::assert_json_snapshot!(tx.sha256_of_proto_encoding());
}

#[test]
fn signed_transaction_verification_roundtrip() {
let signing_key = SigningKey::from([
213, 191, 74, 63, 204, 231, 23, 176, 56, 139, 204, 39, 73, 235, 193, 72, 173, 153, 105,
178, 63, 69, 238, 27, 96, 95, 213, 135, 120, 87, 106, 196,
]);

let transfer = TransferAction {
to: Address::from([0; 20]),
amount: 0,
asset_id: default_native_asset_id(),
fee_asset_id: default_native_asset_id(),
};

let params = TransactionParams {
nonce: 1,
chain_id: "test-1".to_string(),
};
let unsigned = UnsignedTransaction {
actions: vec![transfer.into()],
params,
};

let signed_tx = unsigned.into_signed(&signing_key);
let raw = signed_tx.to_raw();

// `try_from_raw` verifies the signature
SignedTransaction::try_from_raw(raw).unwrap();
}
}
58 changes: 29 additions & 29 deletions ...napshots/astria_core__protocol__transaction__v1alpha1__test__signed_transaction_hash.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,36 @@ source: crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs
expression: tx.sha256_of_proto_encoding()
---
[
45,
183,
116,
196,
217,
243,
23,
192,
66,
217,
1,
108,
44,
224,
168,
98,
240,
215,
197,
83,
178,
71,
115,
206,
104,
180,
159,
248,
74,
224,
209,
176,
241,
29,
14,
237,
215,
243,
132,
7,
196,
102,
232,
226,
192,
10,
114,
218,
129,
58,
199
197,
41,
11,
211,
188,
188,
49,
43,
168,
198
]
3 changes: 2 additions & 1 deletion proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package astria.protocol.transactions.v1alpha1;
import "astria/primitive/v1/types.proto";
import "astria_vendored/penumbra/core/component/ibc/v1/ibc.proto";
import "astria_vendored/tendermint/abci/types.proto";
import "google/protobuf/any.proto";

// `SignedTransaction` is a transaction that has
// been signed by the given public key.
Expand All @@ -13,7 +14,7 @@ import "astria_vendored/tendermint/abci/types.proto";
message SignedTransaction {
bytes signature = 1;
bytes public_key = 2;
UnsignedTransaction transaction = 3;
google.protobuf.Any transaction = 3;
}

// `UnsignedTransaction` is a transaction that does
Expand Down
Loading