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

TRN-609 Improve transact error handling #893

Merged
merged 2 commits into from
Nov 4, 2024
Merged

TRN-609 Improve transact error handling #893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a28ab89
ensure correct InvalidTransaction err is propagated after validation
JCSanPedro Oct 10, 2024
ff7e854
ensure invalid tx is returned for no futurepass account
JCSanPedro Oct 16, 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
86 changes: 45 additions & 41 deletions pallet/doughnut/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,49 +169,53 @@ where
len: usize,
) -> Option<TransactionValidity> {
if let Call::transact { call: inner_call, doughnut, genesis_hash, nonce, tip, .. } = self {
let fee_payer_address = Self::validate_params(doughnut, genesis_hash, inner_call).ok()?;
let sender_address = T::AccountId::from(*origin);

// construct the validation instances
let validations_fee_payer: DoughnutFeePayerValidations<T> =
(ChargeTransactionPayment::<T>::from((*tip).into()),);
let validations_sender: DoughnutSenderValidations<T> =
(CheckNonZeroSender::new(), CheckNonce::from(nonce.clone().into()), CheckWeight::new());

SignedExtension::validate(
&validations_sender,
&sender_address,
&(**inner_call).clone().into(),
dispatch_info,
len,
)
.ok()?;
SignedExtension::validate(
&validations_fee_payer,
&fee_payer_address,
&(**inner_call).clone().into(),
dispatch_info,
len,
)
.ok()?;

// priority is based on the provided tip in the doughnut transaction data
let priority = ChargeTransactionPayment::<T>::get_priority(&dispatch_info, len, (*tip).into(), 0.into());
let who: T::AccountId = (*origin).into();
let account = frame_system::Account::<T>::get(who.clone());
let transaction_nonce = *nonce as u32;
let mut builder =
ValidTransactionBuilder::default().and_provides((origin, transaction_nonce)).priority(priority);

// In the context of the pool, a transaction with
// too high a nonce is still considered valid
if transaction_nonce > account.nonce.clone().into() {
if let Some(prev_nonce) = transaction_nonce.checked_sub(1) {
builder = builder.and_requires((origin, prev_nonce))
let validate = || -> TransactionValidity {
let fee_payer_address = Self::validate_params(doughnut, genesis_hash, inner_call).map_err(|_| {
InvalidTransaction::Call
})?;
let sender_address = T::AccountId::from(*origin);

// construct the validation instances
let validations_fee_payer: DoughnutFeePayerValidations<T> =
(ChargeTransactionPayment::<T>::from((*tip).into()),);
let validations_sender: DoughnutSenderValidations<T> =
(CheckNonZeroSender::new(), CheckNonce::from(nonce.clone().into()), CheckWeight::new());

SignedExtension::validate(
&validations_sender,
&sender_address,
&(**inner_call).clone().into(),
dispatch_info,
len,
)?;
SignedExtension::validate(
&validations_fee_payer,
&fee_payer_address,
&(**inner_call).clone().into(),
dispatch_info,
len,
)?;

// priority is based on the provided tip in the doughnut transaction data
let priority = ChargeTransactionPayment::<T>::get_priority(&dispatch_info, len, (*tip).into(), 0.into());
let who: T::AccountId = (*origin).into();
let account = frame_system::Account::<T>::get(who.clone());
let transaction_nonce = *nonce as u32;
let mut builder =
ValidTransactionBuilder::default().and_provides((origin, transaction_nonce)).priority(priority);

// In the context of the pool, a transaction with
// too high a nonce is still considered valid
if transaction_nonce > account.nonce.clone().into() {
if let Some(prev_nonce) = transaction_nonce.checked_sub(1) {
builder = builder.and_requires((origin, prev_nonce))
}
}
}

Some(builder.build())
builder.build()
};

Some(validate())
} else {
None
}
Expand Down
4 changes: 2 additions & 2 deletions pallet/doughnut/src/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,7 +906,7 @@ fn signed_extension_validations_low_balance_fails() {
tip: 0,
signature: vec![],
};
let outer_signature = holder.sign_ecdsa(&outer_call.encode().as_slice());
let outer_signature = holder.sign_eip191(&outer_call.encode().as_slice());

// validate self contained extrinsic is invalid (invalid signature)
let xt: mock::UncheckedExtrinsicT = fp_self_contained::UncheckedExtrinsic::new_unsigned(
Expand All @@ -928,7 +928,7 @@ fn signed_extension_validations_low_balance_fails() {
xt.clone().into(),
H256::default()
),
TransactionValidityError::Invalid(InvalidTransaction::BadProof)
TransactionValidityError::Invalid(InvalidTransaction::Payment)
);
});
}
Expand Down
91 changes: 47 additions & 44 deletions pallet/xrpl/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,57 +143,60 @@ where
len: usize,
) -> Option<TransactionValidity> {
if let Call::transact { encoded_msg, signature, call } = self {
let (nonce, tip) = validate_params::<T>(encoded_msg.as_bytes_ref(), signature.as_bytes_ref(), call)
.map_err(|e| {
log!(info, "⛔️ validate_self_contained: failed to validate params: {:?}", e);
InvalidTransaction::Call
})
.ok()?;

let validations: XRPLValidations<T> = (
CheckNonZeroSender::new(),
CheckSpecVersion::<T>::new(),
CheckTxVersion::<T>::new(),
CheckEra::<T>::from(Era::immortal()),
CheckWeight::new(),
ChargeTransactionPayment::<T>::from(tip.into()),
);
let validate = || -> TransactionValidity {
let (nonce, tip) = validate_params::<T>(encoded_msg.as_bytes_ref(), signature.as_bytes_ref(), call)
.map_err(|e| {
log!(info, "⛔️ validate_self_contained: failed to validate params: {:?}", e);
InvalidTransaction::Call
})?;

let validations: XRPLValidations<T> = (
CheckNonZeroSender::new(),
CheckSpecVersion::<T>::new(),
CheckTxVersion::<T>::new(),
CheckEra::<T>::from(Era::immortal()),
CheckWeight::new(),
ChargeTransactionPayment::<T>::from(tip.into()),
);

let mut tx_origin = T::AccountId::from(*origin);
let mut tx_origin = T::AccountId::from(*origin);

// validate signed extensions using origin
SignedExtension::validate(&validations, &tx_origin, &(*call.clone()).into(), dispatch_info, len).ok()?;
// validate signed extensions using origin
SignedExtension::validate(&validations, &tx_origin, &(*call.clone()).into(), dispatch_info, len)?;

// validate signed extensions using EOA - for futurepass based transactions
if <T as pallet::Config>::FuturepassLookup::check_extrinsic(&call, &()) {
// this implies that the origin is futurepass address; we need to get the EOA associated with it
let eoa = <T as pallet::Config>::FuturepassLookup::unlookup(*origin);
if eoa == H160::zero() {
log!(info, "⛔️ failed to get EOA from futurepass address");
return None;
// validate signed extensions using EOA - for futurepass based transactions
if <T as pallet::Config>::FuturepassLookup::check_extrinsic(&call, &()) {
// this implies that the origin is futurepass address; we need to get the EOA associated with it
let eoa = <T as pallet::Config>::FuturepassLookup::unlookup(*origin);
if eoa == H160::zero() {
log!(info, "⛔️ failed to get EOA from futurepass address");
return Err(TransactionValidityError::Invalid(InvalidTransaction::BadProof));
}
tx_origin = T::AccountId::from(eoa);
}
tx_origin = T::AccountId::from(eoa);
}

// validate nonce signed extension using EOA
let validations: EOANonceValidation<T> = (CheckNonce::from(nonce.into()),);
SignedExtension::validate(&validations, &tx_origin, &(*call.clone()).into(), dispatch_info, len).ok()?;

// priority is based on the provided tip in the xrpl transaction data
let priority = ChargeTransactionPayment::<T>::get_priority(&dispatch_info, len, tip.into(), 0.into());
let who: T::AccountId = (tx_origin).clone().into();
let account = frame_system::Account::<T>::get(who);
let mut builder =
ValidTransactionBuilder::default().and_provides((tx_origin.clone(), nonce)).priority(priority);

// in the context of the pool, a transaction with too high a nonce is still considered valid
if nonce > account.nonce.into() {
if let Some(prev_nonce) = nonce.checked_sub(1) {
builder = builder.and_requires((tx_origin, prev_nonce))
// validate nonce signed extension using EOA
let validations: EOANonceValidation<T> = (CheckNonce::from(nonce.into()),);
SignedExtension::validate(&validations, &tx_origin, &(*call.clone()).into(), dispatch_info, len)?;

// priority is based on the provided tip in the xrpl transaction data
let priority = ChargeTransactionPayment::<T>::get_priority(&dispatch_info, len, tip.into(), 0.into());
let who: T::AccountId = (tx_origin).clone().into();
let account = frame_system::Account::<T>::get(who);
let mut builder =
ValidTransactionBuilder::default().and_provides((tx_origin.clone(), nonce)).priority(priority);

// in the context of the pool, a transaction with too high a nonce is still considered valid
if nonce > account.nonce.into() {
if let Some(prev_nonce) = nonce.checked_sub(1) {
builder = builder.and_requires((tx_origin, prev_nonce))
}
}
}

Some(builder.build())
builder.build()
};

Some(validate())
} else {
None
}
Expand Down
18 changes: 9 additions & 9 deletions pallet/xrpl/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ mod self_contained_call {
}));
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Call),
);
});
}
Expand Down Expand Up @@ -115,7 +115,7 @@ mod self_contained_call {
}));
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Call),
);
});
}
Expand All @@ -139,7 +139,7 @@ mod self_contained_call {
}));
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.clone().into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Call),
);

// validate self contained extrinsic is invalid (invalid signature)
Expand All @@ -150,7 +150,7 @@ mod self_contained_call {
}));
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.clone().into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Call),
);

// validate self contained extrinsic fails, user does not have funds to pay for transaction (corrected signature)
Expand All @@ -161,7 +161,7 @@ mod self_contained_call {
}));
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.clone().into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Payment),
);
// validate same transaction is successful after funding caller
let tx = XRPLTransaction::try_from(tx_bytes.as_bytes_ref()).unwrap();
Expand Down Expand Up @@ -201,7 +201,7 @@ mod self_contained_call {
System::set_block_number(10);
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.clone().into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Call),
);

// reset block number, extrinsic validation should pass now
Expand Down Expand Up @@ -241,7 +241,7 @@ mod self_contained_call {
// validate the same extrinsic will fail (nonce mismatch) - preventing replays
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.clone().into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Stale),
);
});
}
Expand Down Expand Up @@ -275,7 +275,7 @@ mod self_contained_call {
System::set_block_number(10);
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.clone().into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Call),
);

// reset block number, extrinsic validation should pass now
Expand Down Expand Up @@ -315,7 +315,7 @@ mod self_contained_call {
// validate the same extrinsic will fail (nonce mismatch) - preventing replays
assert_err!(
Executive::validate_transaction(TransactionSource::External, xt.clone().into(), H256::default()),
TransactionValidityError::Invalid(InvalidTransaction::BadProof),
TransactionValidityError::Invalid(InvalidTransaction::Stale),
);
});
}
Expand Down
Loading