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

bug: decrementing 0 should fail, but it's getting successful transaction #683

Closed
Tracked by #324
marciob opened this issue Aug 16, 2023 · 5 comments · Fixed by #824
Closed
Tracked by #324

bug: decrementing 0 should fail, but it's getting successful transaction #683

marciob opened this issue Aug 16, 2023 · 5 comments · Fixed by #824

Comments

@marciob
Copy link

marciob commented Aug 16, 2023

Bug Report

Kakarot version:

when decrementing 0 it normally should get a failed transaction, but actually it is getting a successful transaction.

Current behavior:

while testing that decrement function with a count state 0:

    // Function to decrement count by 1
    function dec() public {
        // This function will fail if count = 0
        count -= 1;
    }

when trying to decrementing from the 0 value, although the state value still remains 0, it is getting a successful transaction, it should fail.

Expected behavior:

when trying to decrementing from the 0 value, the txn should revert with an error like that:

transact to Counter.dec errored: VM error: revert.

revert
	The transaction has been reverted to the initial state.
Note: The called function should be payable if you send value and the value you send should be less than your current balance.
Debug the transaction to get more information.

Steps to reproduce:

I used that contract:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

contract Counter {
    uint public count;

    // Function to get the current count
    function get() public view returns (uint) {
        return count;
    }

    // Function to increment count by 1
    function inc() public {
        count += 1;
    }

    // Function to decrement count by 1
    function dec() public {
        // This function will fail if count = 0
        count -= 1;
    }
}

at the initial state, with count at 0, I tried to call dec(), it got a successful transaction.

Other information:

EVM environment: Kakarot ZKEVM blockchain (Testnet)
Solidity version: 0.8.17
No balance was sent to the contract upon deployment.

@Eikix
Copy link
Member

Eikix commented Aug 16, 2023

why does the note talk about value and payable?
did you try to reproduce on the normal remix evm?

@marciob
Copy link
Author

marciob commented Aug 16, 2023

@Eikix I am testing from the "Solidity-by-example" contracts examples, the comments in the contract are from there.

Yes, I tried to reproduce on normal remix, the "Expected behavior" mentioned is from the normal remix evm.

@marciob
Copy link
Author

marciob commented Aug 16, 2023

also, the "payable" note from remix I guess it's a default note in some errors, just in case.

@ClementWalter
Copy link
Member

I remember that we once used safe_uint256 that was actually raising, but we found out that it was normally the solidity compiler that adds the safe checks and revert and not the SUB opcode.

Actually, running in the playground I do see that it's reverting, but after GT on the result of SUB, not sure exactly what happens for us though. We could add a test into test_counter

@ClementWalter
Copy link
Member

just tried rn adding a test to test_counter.py and it doesn't REVERT.

See #767

@ClementWalter ClementWalter mentioned this issue Nov 23, 2023
7 tasks
ClementWalter added a commit that referenced this issue Nov 23, 2023
Time spent on this PR: 0.1

## Pull request type

Please check the type of change your PR introduces:

- [ ] Bugfix
- [ ] Feature
- [x] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe):

## What is the current behavior?

A test is marked as failed while it has the expected behavior
No test for safe sub.

Resolves #683

## What is the new behavior?

Fixed tests.
Behavior was already correct.
@github-project-automation github-project-automation bot moved this from 🆕 Backlog to ✅ Done in Kakarot on Starknet Nov 23, 2023
matthieuauger pushed a commit to matthieuauger/kakarot that referenced this issue Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants