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

Fixed an issue where cancelling NFT offer did not cancel other offers… #19129

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

ChiaMineJP
Copy link
Contributor

Fixing Chia-Network/chia-blockchain-gui#2563

I found that the primary issues of the above case are:

  1. Canceling an offer with a NFT in offering-side does not set PENDING_CANCEL status for trades which also offers the same NFT.
  • This is because current wallet implementation updates trade status by trade_id and not by coins being spent.
  1. Confirming cancellation does not set CANCELLED status to the trade records.
  • I understood the root cause. I'll desribe it in the following paragraph.

Why cancelling multiple NFT offers doesn't work

When confirming the offer cancellation, which means the offering NFT coin was spent but requesting coins were not created, trade_records wallet db table in the issue's situation would be like:

trade_id status
offer1 PENDING_ACCEPT
offer2 PENDING_ACCEPT
offer3 PENDING_CANCEL

(This inconsistent statuses is caused by issue 1. above.)

Then look at the code below. This code is executed when monitoring coin state has been changed.
For example, the offering coin is spent to cancel.
https://github.com/Chia-Network/chia-blockchain/blob/2.5.0/chia/wallet/trade_manager.py#L154
image

The target trade whose status is expected to be set to CANCELLED is acquired by self.get_trade_by_coin(coin_state.coin).
This is the problem.
When this is executed, it gets offer1 in the above table.
offer1's status is PENDING_ACCEPT.
When offer is cancelled, its status must be PENDING_CANCEL according to
https://github.com/Chia-Network/chia-blockchain/blob/2.5.0/chia/wallet/trade_manager.py#L204
image

So the current implementation misses to recognize offer cancellation for NFT.
This is the PR to fix this issue.

@ChiaMineJP ChiaMineJP requested a review from a team as a code owner January 13, 2025 14:28
@ChiaMineJP ChiaMineJP self-assigned this Jan 13, 2025
@ChiaMineJP ChiaMineJP added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Jan 13, 2025
@ChiaMineJP ChiaMineJP force-pushed the cmj/fix-nft-offer-cancellation branch from d3fd024 to 5cbd747 Compare January 13, 2025 14:50
@Quexington
Copy link
Contributor

Just as a note, this is techincally a hack to cover up the fact that we're allowing users to create more than one offer of the same coin.

@Rigidity
Copy link
Contributor

Just as a note, this is techincally a hack to cover up the fact that we're allowing users to create more than one offer of the same coin.

It's desirable to allow this, if you want to request multiple different assets as options.

Copy link
Contributor

File Coverage Missing Lines
chia/wallet/trade_manager.py 93.6% lines 158-159, 161
Total Missing Coverage
53 lines 3 lines 94%

@Quexington
Copy link
Contributor

Just as a note, this is techincally a hack to cover up the fact that we're allowing users to create more than one offer of the same coin.

It's desirable to allow this, if you want to request multiple different assets as options.

Well that's a hack too for not supporting this functionality natively

@pmaslana pmaslana merged commit 3e8b3b5 into main Jan 13, 2025
364 of 365 checks passed
@pmaslana pmaslana deleted the cmj/fix-nft-offer-cancellation branch January 13, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage-diff Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants