Clarification on test_transfer_from_doesnt_consume_infinite_allowance and BoundedInt::max() Usage

Support Contracts
, ,
1

Hello OpenZeppelin community,

I am currently working on a project and came across the test_transfer_from_doesnt_consume_infinite_allowance test case in one of your test suites. I noticed that BoundedInt::max() is used to set an allowance, and the test checks that the allowance remains unchanged after a transferFrom operation.

Initially, I assumed that BoundedInt::max() represents a very large, but finite number. However, the test seems to imply that it represents an infinite allowance. When I adjusted the test to check for BoundedInt::max() - VALUE, it passed, which aligns with my initial understanding.

Could you please clarify if BoundedInt::max() is intended to represent an infinite allowance in this context? Also, is the test designed to ensure that an infinite allowance remains unaffected by transferFrom operations?

#[test]
#[available_gas(2000000)]
fn test_transfer_from_doesnt_consume_infinite_allowance() {
    let mut state = setup();
    testing::set_caller_address(OWNER());
    ERC20Impl::approve(ref state, SPENDER(), BoundedInt::max());

    testing::set_caller_address(SPENDER());
    ERC20Impl::transfer_from(ref state, OWNER(), RECIPIENT(), VALUE);

    assert(
        ERC20Impl::allowance(@state, OWNER(), SPENDER()) == BoundedInt::max(),
        'Allowance should not change'
    );
}
2

Hi @henryf10h,

Initially, I assumed that BoundedInt::max() represents a very large, but finite number

It does represent a finite number, in this case being 2**256-1, that is the u256 max value, the test works intentionally as expected by not consuming allowance, because in the _spend_allowance function, in the ERC20 implementation, there's a specific condition designed for this.

Could you please clarify if BoundedInt::max() is intended to represent an infinite allowance in this context? Also, is the test designed to ensure that an infinite allowance remains unaffected by transferFrom operations?

It is intended to represent an infinite allowance in this context, and the test is designed for that yes.

Hope that helps.

3 Likes
3

Hey @henryf10h,

Thank you for bringing this up!

When I adjusted the test to check for BoundedInt::max() - VALUE , it passed, which aligns with my initial understanding.

You mean in the assertion? Would you mind further clarifying how you adjusted the test?

Could you please clarify if BoundedInt::max() is intended to represent an infinite allowance in this context?

BoundedInt::max() itself is a finite number; however, when it’s used as an allowance value in the ERC20 context (through approve), it indeed represents an infinite allowance. The infinite allowance can save gas especially when multiple transfers of a token occur because each transfer will neither write a new allowance value in storage nor fire an Approval event.

Also, is the test designed to ensure that an infinite allowance remains unaffected by transferFrom operations?

Yes, exactly

3 Likes
4

Sounds to me like he or she have replaced the two occurrences of BoundedInt::max() with BoundedInt::max() - VALUE or with BoundedInt::max().sub(VALUE), which indeed raises some doubts regarding the correctness of the test (which should NOT have passed following this change).

In fact, the test should not pass even if you replace only one of these two occurrences, so something is definitely off here - either the contract, or the test, or the description of the problem at hand (most likely).

1 Like
5

In fact, the test should not pass even if you replace only one of these two occurrences, so something is definitely off here - either the contract, or the test, or the description of the problem at hand (most likely).

mmm, that's weird, I did try these combinations and they all successfully failed as they should. I cannot reproduce what you are presenting.

1 Like
6

Hello @ericng, @barakman, @andrewfleming,

I appreciate the insight provided by all. I realize now that I didn’t provide a full picture of my project initially, and I apologize for any confusion. I am working on adapting an ERC20 token into a reflective/deflationary model using Cairo, drawing inspiration from your ERC20 standard and tests. However, to maintain consistency with the original reflect.sol codebase, I didn’t incorporate the _spend_allowance function, and thus, in my logic, the allowance gets fully utilized post each transfer_from operation, contrary to having an infinite allowance scenario.

I assumed a full consumption of allowance, overlooking the specific logic used in your implementation. I apologize for the oversight and any ambiguity in my earlier communication. Thank you for bringing this to light and aiding in clarifying.

Thank you once more for your time and feedback. Your responses have been invaluable for me to keep the progress in testing.

Here is the repository if you would like to take a look:

1 Like