Add transactions related scenarios #790
Conversation
| pub fn vote_cast_fees(constant_fee: u64, coefficient: u64, certificate: u64) { | ||
| let expected_fees = constant_fee + coefficient + certificate; | ||
| let favorable = Choice::new(1); | ||
| let voting_token = TokenName::try_from(vec![0u8; TOKEN_NAME_MAX_SIZE]).unwrap(); |
There was a problem hiding this comment.
i think we got TestGen::token_name() static method for this purpose
There was a problem hiding this comment.
Not working for this purpose as TestGen::token_name() returns an incompatible type of token.
As discussed I created a ticket to track this desiderata: https://input-output.atlassian.net/browse/NPG-1217
| } | ||
|
|
||
| #[quickcheck] | ||
| pub fn vote_cast_fees(constant_fee: u64, coefficient: u64, certificate: u64) { |
There was a problem hiding this comment.
I'm pretty sure that LinearFee implements Arbitrary, so you can write directly:
pub fn vote_cast_fees(linear_fee: LinearFee) {
|
|
||
| LedgerStateVerifier::new(ledger.into()) | ||
| .info("account balance is correct") | ||
| .address_has_expected_balance(alice.as_account_data(), Value(1000 - expected_fees)); |
There was a problem hiding this comment.
It would be better to assign some variable to express 1000. Because when reading test it's not 100% clear that this 1000 is initial account balance or initial rewards
There was a problem hiding this comment.
And it would also be better to have different values for :
- rewards
- voting tokens
- balance
to avoid any confusion
| } | ||
|
|
||
| #[quickcheck] | ||
| pub fn vote_tally_fees(constant_fee: u64, coefficient: u64, certificate: u64) { |
There was a problem hiding this comment.
the same as in line 254. you can use LinearFee
|
|
||
| #[quickcheck] | ||
| pub fn vote_tally_fees(constant_fee: u64, coefficient: u64, certificate: u64) { | ||
| let expected_fees = constant_fee + coefficient + certificate; |
There was a problem hiding this comment.
And as a result expected expected_fees can be calculated :
let expected_fees = linear_fee.constant + linear_fee.coefficient + linear_fee.certificate
There was a problem hiding this comment.
not working as is, implemented as:
let expected_fee = linear_fee.calculate(None, 1, 1)
after review
|
|
||
| LedgerStateVerifier::new(ledger.into()) | ||
| .info("account balance is correct") | ||
| .address_has_expected_balance(alice.as_account_data(), Value(1000 - expected_fees)); |
There was a problem hiding this comment.
the same as comment in line 292
| LedgerStateVerifier::new(ledger.into()) | ||
| .address_has_expected_balance(bob.as_account_data(), Value(bob_initial_balance + amount)); | ||
|
|
||
| TestResult::passed() |
There was a problem hiding this comment.
i think, you don't need to explicitly set TestResult to pass. It would be pass automatically if none of assertion failed etc.
| let (mut ledger, controller) = prepare_scenario() | ||
| .with_config( | ||
| ConfigBuilder::new() | ||
| .with_discrimination(Discrimination::Test) |
There was a problem hiding this comment.
is test discrimination important for this test case? If not, it's better to remove it, because reader can think that it has something to do with fees
| .transfer_funds(&alice, &bob, &mut ledger, amount + total_fees) | ||
| .unwrap(); | ||
|
|
||
| TestResult::from_bool( |
There was a problem hiding this comment.
It would be good to add check that alice bob and fees are the same after and before this transaction, as test cases says validate state after invalid transaction
There was a problem hiding this comment.
As discussed, fee checks are already covered by tests in
chain-impl-mockchain/src/testing/e2e/fee.rs
| return TestResult::discard(); | ||
| }; | ||
|
|
||
| let mut rng = rand::thread_rng(); |
There was a problem hiding this comment.
it is not very quickcheckish way of getting random number. I would write a bit more code but it would be nicely incorporated into framework.
First let's create wrapping struct for u64 with value constraint (1..10):
struct Random1to10(u64);
and then implement arbitrary
impl Arbitrary for Random1to10 {
fn arbitrary<G: Gen>(g: &mut G) -> Self {
Self(u64::arbitrary(g) % 10 + 1)
}
}
Finally use it in quickcheck:
pub fn validate_ledger_state_after_transaction(amount: u64, random: Random1to10 ) -> TestResult {
|
|
||
| let (fee, total_fees) = (LinearFee::new(1, 1, 1), 3); | ||
| let (alice_initial_balance, bob_initial_balance) = | ||
| (amount + total_fees + 1, rng.gen_range(1..10)); |
There was a problem hiding this comment.
magic number (1), please name it so we know why it's 1
|
|
||
| let mut rng = rand::thread_rng(); | ||
|
|
||
| let (fee, total_fees) = (LinearFee::new(1, 1, 1), 3); |
There was a problem hiding this comment.
maybe linear fee would also be arbitrary value ? and another magic number : 3
|
|
||
| let mut rng = rand::thread_rng(); | ||
|
|
||
| let (fee, total_fees) = (LinearFee::new(1, 1, 1), 3); |
There was a problem hiding this comment.
the same as in method above:
- linear fees
- random arbitrary number
- no magic numbers
|
Overall code looks good. It just need to have some readability improvements |
|
|
||
| #[quickcheck] | ||
| pub fn validate_ledger_state_after_invalid_transaction(amount: Random1to10, linear_fee: LinearFee) { | ||
| let total_fees = linear_fee.constant + linear_fee.coefficient + linear_fee.certificate; |
There was a problem hiding this comment.
Shouldn't this be the same like in LINE: 18
* added ledger status validation after transaction scenario * removed useless validation * added scenario for invalid transaction * added scenarios for correct fees when casting & tallying votes * partial PR correction * PR corrections * deleted comment * PR corrections
* update rlp crate, expose it via chain-evm * add ethereum crate to cargo manifest in chain-evm * rlp encoding for evm transactions update tests for evm transaction serialization bijection * add AccessList type and impl rlp en/de updates tests and vm function signatures * impl rlp::Encodable for EvmTransaction * use AccessList and AccessListItem types from ethereum crate replaces handmade vec of tuples * implement Encodable/Decodabe traits for EvmTransaction adds test for RLP bijection, ignores previous bijection test * expose chain_evm::Config thru chain-impl-mockchain * implement RLP encoding under the hood for Payload/DeserializeFromSlice uses common traits for fragment ser/de with RLP encoding for EVM transactions * clean up lints, make clippy happy * add feature gate for evm serialize_in function * clean clippy lint * move import used only in tests * make utility function for access list conversation into private * impl Serialize n Deserialize for EvmTransaction remove DeserializeFromSlice, as we get a blanket impl now * update evm transaction serialization bijection test * more test coverage to evm transaction RLP ser/de * serialize length of RLP bytes Co-authored-by: Enzo Cioppettini <48031343+enzoc4@users.noreply.github.com> * serialize length of RLP bytes in Serialize trait Co-authored-by: Enzo Cioppettini <48031343+enzoc4@users.noreply.github.com> * read expected RLP bytes from Codec Co-authored-by: Enzo Cioppettini <48031343+enzoc4@users.noreply.github.com> * test no longer applies * arbitrary AccessList is either empty or has 4 items Co-authored-by: Alex Pozhylenkov <leshiy12345678@gmail.com> * add EthereumTransaction type implements RLP encoding/decoding implements Serialize/Deserialize from chain_core * add unsigned transaction types, implement RLP decoding brings in secp256k1 crate dependency * adds signed and unsigned tx support for Ethereum includes tests from https://eips.ethereum.org/EIPS/eip-155#example * fix clippy errors * sign and recover transactions also adds tests for the Secret type, which holds a SecretKey and offers helpful conversions * repair signed tx recovery, improve tests * few corrections to doc strings * refactor signed tx recovery, tidy up code a bit * Add transactions related scenarios (#790) * added ledger status validation after transaction scenario * removed useless validation * added scenario for invalid transaction * added scenarios for correct fees when casting & tallying votes * partial PR correction * PR corrections * deleted comment * PR corrections * change Vec<u8> to Box<[u8]> (#803) * cleanup code, add docs, handle errors * update EvmTransaction encoding for Box[u8] bytecode Co-authored-by: Enzo Cioppettini <48031343+enzoc4@users.noreply.github.com> Co-authored-by: Alex Pozhylenkov <leshiy12345678@gmail.com> Co-authored-by: Nicolo' Padovani <69471887+nicopado@users.noreply.github.com>
Adding scenario tests:
valid transaction
invalid transaction
correct fees when casting votes
correct fees when tallying