-
Notifications
You must be signed in to change notification settings - Fork 287
fix: update is_approved in _add_token_transfer accumulation branch #2290
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
base: main
Are you sure you want to change the base?
Changes from all commits
34cedca
98b2ca3
fc23cbc
e6bc36d
70ed965
99a851e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,7 +152,7 @@ def _add_token_transfer( | |
| raise TypeError("is_approved must be a boolean.") | ||
|
|
||
| for transfer in self.token_transfers[token_id]: | ||
| if transfer.account_id == account_id: | ||
| if transfer.account_id == account_id and transfer.is_approved == is_approved: | ||
| transfer.amount += amount | ||
| transfer.expected_decimals = expected_decimals | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should avoid this being overwritten by none eg |
||
| return | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -453,7 +453,7 @@ def test_approved_token_transfer_with_decimals(mock_account_ids): | |
|
|
||
|
|
||
| def test_approved_token_transfer_accumulation(mock_account_ids): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can add a test for the new features def test_merge_preserves_expected_decimals(mock_account_ids): |
||
| """Test that approved token transfers accumulate for the same account.""" | ||
| """Test that approved token transfers are stored as separate entries from normal ones.""" | ||
| account_id_1, account_id_2, _, token_id_1, _ = mock_account_ids | ||
| transfer_tx = TransferTransaction() | ||
|
|
||
|
|
@@ -471,18 +471,76 @@ def test_approved_token_transfer_accumulation(mock_account_ids): | |
| assert transfer_2.is_approved is False | ||
| assert transfer_2.expected_decimals is None | ||
|
|
||
| # Add approved transfer with decimals for account_1 (accumulates) | ||
| # Add approved transfer with decimals for account_1 (separate entry, not merged) | ||
| transfer_tx.add_approved_token_transfer_with_decimals(token_id_1, account_id_1, 200, 8) | ||
|
|
||
| # Verify accumulation | ||
| transfer_1 = transfer_tx.token_transfers[token_id_1][0] | ||
| transfer_2 = transfer_tx.token_transfers[token_id_1][1] | ||
| assert transfer_1.amount == 700 # 500 + 200 | ||
| assert transfer_1.is_approved is False # unchanged | ||
| assert transfer_1.expected_decimals == 8 # updated from the accumulation | ||
| assert transfer_2.amount == 300 # unchanged | ||
| assert transfer_2.is_approved is False # unchanged | ||
| assert transfer_2.expected_decimals is None # unchanged | ||
| # Verify stored as separate entries | ||
| transfers = transfer_tx.token_transfers[token_id_1] | ||
| assert len(transfers) == 3 # account_1 normal, account_2 normal, account_1 approved | ||
|
|
||
| assert transfers[0].amount == 500 # unchanged | ||
| assert transfers[0].is_approved is False # unchanged | ||
| assert transfers[1].amount == 300 # unchanged | ||
| assert transfers[1].is_approved is False # unchanged | ||
| assert transfers[2].amount == 200 | ||
| assert transfers[2].is_approved is True | ||
| assert transfers[2].expected_decimals == 8 | ||
|
|
||
|
|
||
| def test_normal_and_approved_transfers_kept_separate(mock_account_ids): | ||
| """Normal and approved transfers for the same account are stored as separate entries.""" | ||
| account_id_1, account_id_2, _, token_id_1, _ = mock_account_ids | ||
| transfer_tx = TransferTransaction() | ||
|
|
||
| transfer_tx.add_token_transfer(token_id_1, account_id_1, 500) | ||
| transfer_tx.add_token_transfer(token_id_1, account_id_2, -500) | ||
| transfer_tx.add_approved_token_transfer(token_id_1, account_id_1, 200) | ||
| transfer_tx.add_token_transfer(token_id_1, account_id_2, -200) | ||
|
|
||
| transfers = transfer_tx.token_transfers[token_id_1] | ||
| assert len(transfers) == 3 # account_1 normal, account_2 accumulated, account_1 approved | ||
|
|
||
| assert transfers[0].amount == 500 | ||
| assert transfers[0].is_approved is False | ||
|
|
||
| assert transfers[1].amount == -700 # -500 + -200 accumulated | ||
| assert transfers[1].is_approved is False | ||
|
|
||
| assert transfers[2].amount == 200 | ||
| assert transfers[2].is_approved is True | ||
|
|
||
|
|
||
| def test_same_approved_transfers_accumulate(mock_account_ids): | ||
| """Two approved transfers for the same account DO accumulate.""" | ||
| account_id_1, account_id_2, _, token_id_1, _ = mock_account_ids | ||
| transfer_tx = TransferTransaction() | ||
|
|
||
| transfer_tx.add_approved_token_transfer(token_id_1, account_id_1, 300) | ||
| transfer_tx.add_approved_token_transfer(token_id_1, account_id_1, 200) | ||
| transfer_tx.add_token_transfer(token_id_1, account_id_2, -500) | ||
|
|
||
| transfers = transfer_tx.token_transfers[token_id_1] | ||
| assert len(transfers) == 2 # account_1 approved (merged), account_2 normal | ||
|
|
||
| assert transfers[0].amount == 500 # 300 + 200 accumulated | ||
| assert transfers[0].is_approved is True | ||
|
|
||
| assert transfers[1].amount == -500 | ||
| assert transfers[1].is_approved is False | ||
|
|
||
|
|
||
| def test_add_approved_token_transfer_no_decimals(mock_account_ids): | ||
| """add_approved_token_transfer (non-decimal variant) sets is_approved=True.""" | ||
| account_id_1, account_id_2, _, token_id_1, _ = mock_account_ids | ||
| transfer_tx = TransferTransaction() | ||
|
|
||
| transfer_tx.add_approved_token_transfer(token_id_1, account_id_1, -1000) | ||
| transfer_tx.add_token_transfer(token_id_1, account_id_2, 1000) | ||
|
|
||
| transfer = transfer_tx.token_transfers[token_id_1][0] | ||
| assert transfer.amount == -1000 | ||
| assert transfer.is_approved is True | ||
| assert transfer.expected_decimals is None | ||
|
|
||
|
|
||
| def test_approved_token_transfer_validation(mock_account_ids): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, these changes caused the
e2e test tests/integration/transfer_transaction_e2e_test.py::test_integration_transfer_transaction_approved_token_transferto fail withACCOUNT_REPEATED_IN_ACCOUNT_AMOUNTS.This happens cause fro single token within one
TransferTransactionyou cannot have both a standard transfer entry and an approved transfer entry for the same (tokenId, accountId) pair they are treated as separate entries and are not merged.One workaround is to keep the previous behavior, where both transfers are merged into a single entry (either fully standard or fully approved). Since the approval flag is only set once, the later value is effectively ignored.
The other option is to update the e2e test to completely use the standard transfer instead.
Would like to get an opinion from @exploreriii on which direction makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left question in DM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aceppaluni @manishdait what change i have to do in this ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can update the e2e test so it models the approved flow behaviour correctly. The allowance spend is the approved debit from the owner; the credit to the spender is a standard transfer. The same (token, account) pair cannot carry both an approved and a standard entry in one transaction.