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

executor: fix full exit nft #220

Merged
merged 1 commit into from
Oct 12, 2022
Merged

executor: fix full exit nft #220

merged 1 commit into from
Oct 12, 2022

Conversation

keefel
Copy link
Contributor

@keefel keefel commented Oct 11, 2022

Description

Fix full exit nft executor.

Rationale

There are two cases we should exit an empty nft:

  1. The nft index doesn't exist
  2. The nft owner is not the current account
    Both these two cases we should handle correctly, but the earlier logic isn't correct, so fix it.

Example

NA

Changes

Notable changes:

  • NA

Note

/update-integration-keyfile

@keefel keefel added bug Something isn't working ready for review ready for review labels Oct 11, 2022
Copy link
Contributor

@lightning-li lightning-li left a comment

Choose a reason for hiding this comment

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

LGTM

return nil
}

func (e *FullExitNftExecutor) VerifyInputs(skipGasAmtChk bool) error {
bc := e.bc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this because FullExitNft should always be executed? If that is the case, can you take some time to check other executor that does unnecessary verify, and fix it in another new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will.

@keefel keefel merged commit 5050fa5 into bnb-chain:develop Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready for review ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants