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.

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

Refactor coin-based contract instruction tests #779

Merged
merged 7 commits into from
Jun 24, 2024
Merged

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Jun 19, 2024

Work towards #746

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests

Before requesting review

  • I have reviewed the code myself

@Dentosal Dentosal self-assigned this Jun 19, 2024
@Dentosal Dentosal added the no changelog Skips the CI changelog check label Jun 19, 2024
@Dentosal Dentosal marked this pull request as ready for review June 19, 2024 10:18
@Dentosal Dentosal changed the title Add new tests for coin-based contract opcodes Refactpr coin-based contract instruction tests Jun 19, 2024
@Dentosal Dentosal requested a review from a team June 19, 2024 10:19
@Dentosal Dentosal changed the title Refactpr coin-based contract instruction tests Refactor coin-based contract instruction tests Jun 19, 2024
@xgreenx xgreenx requested a review from MitchTurner June 19, 2024 17:40
fuel-vm/src/tests/coins.rs Show resolved Hide resolved
fuel-vm/src/tests/coins.rs Outdated Show resolved Hide resolved
#[test_case(vec![(op::burn, 1, 0)] => RunResult::Panic(PanicReason::NotEnoughBalance); "Burn nonexisting 1")]
#[test_case(vec![(op::burn, Word::MAX, 0)] => RunResult::Panic(PanicReason::NotEnoughBalance); "Burn nonexisting Word::MAX")]
#[test_case(vec![(op::mint, Word::MAX, 0), (op::mint, 1, 0)] => RunResult::Panic(PanicReason::BalanceOverflow); "Mint overflow")]
#[test_case(vec![(op::mint, Word::MAX, 0), (op::burn, 1, 0), (op::mint, 2, 0)] => RunResult::Panic(PanicReason::BalanceOverflow); "Mint,Burn,Mint overflow")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, maybe we want to have just separate bunch of tests for mint and burn to test the overflow of the SubId.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is that? Are there more cases you'd like to test?

fuel-vm/src/tests/coins.rs Outdated Show resolved Hide resolved
#[test_case(1, 0, 10, 5 => RunResult::Panic(PanicReason::TransferZeroCoins); "Cannot transfer 0 coins to non-empty other")]
#[test_case(0, 0, 10, 0 => RunResult::Panic(PanicReason::TransferZeroCoins); "Cannot transfer 0 coins to self")]
#[test_case(1, 1, 10, 0 => RunResult::Success((9, 1)); "Can transfer 1 coins to other")]
#[test_case(0, 1, 10, 0 => RunResult::Success((10, 0)); "Can transfer 1 coins to self")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems the self transfers doesn't affect values in Success. Maybe we want to list all balances of all contract there for clarity of the expected result.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand. The balance must stay the same, as the balance is decreased and then increased again by the same amount. What other balances would help?

}

#[test_case(None, None => RunResult::Success(()); "Normal case works")]
#[test_case(Some(Word::MAX - 31), None => RunResult::Panic(PanicReason::MemoryOverflow); "$rA + 32 overflows")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to return MemoryOverflow instead of ArifmethicOverflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. MemoryOverflow is what happens when an operation conceptually exceeds VM_MAX_RAM. The behavior is quite consistent; ArithmeticOverflow is only used by the ALU operations when they overflow. For instance, we never panic with ArithmeticOverflow when WRAPPING flag is set.

fuel-vm/src/tests/coins.rs Outdated Show resolved Hide resolved
fuel-vm/src/tests/coins.rs Show resolved Hide resolved
fuel-vm/src/tests/coins.rs Show resolved Hide resolved
@Dentosal Dentosal mentioned this pull request Jun 24, 2024
12 tasks
@Dentosal Dentosal added this pull request to the merge queue Jun 24, 2024
Merged via the queue into master with commit 56d7deb Jun 24, 2024
39 checks passed
@Dentosal Dentosal deleted the dento/coin-op-tests branch June 24, 2024 16:58
@xgreenx xgreenx mentioned this pull request Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Skips the CI changelog check testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants