Skip to content

mintNFT Bugfix#2

Open
smckisson wants to merge 3 commits into
buildspace:mainfrom
smckisson:patch-1
Open

mintNFT Bugfix#2
smckisson wants to merge 3 commits into
buildspace:mainfrom
smckisson:patch-1

Conversation

@smckisson

Copy link
Copy Markdown

Change NFT minting block to wrap state var logic inside .then instead of .finally. Code will still be called and set hasClaimedNFT to true even if minting fails when in a finally block.

Change NFT minting block to wrap state var logic inside .then instead of .finally. Code will still be called and set hasClaimedNFT to true even if minting fails when in a finally block.
@smckisson smckisson changed the title mintNFT BugFix mintNFT Bugfix Dec 20, 2021

@nachoiacovino nachoiacovino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

setIsClaiming should go inside the finally since we want if it fails we also want to setIsClaiming to false!

@smckisson

Copy link
Copy Markdown
Author

setIsClaiming should go inside the finally since we want if it fails we also want to setIsClaiming to false!

You're thinking keep the finally but only call setIsClaiming there? If it fails, setIsClaiming is called inside the catch, but could definitely be simplified to only call it once inside a finally

@nachoiacovino nachoiacovino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

exactly! lgtm

@nachoiacovino nachoiacovino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there's an error here, and the error got into the content, you need to delete the semicolon on line 374/368.

Can you also add a PR to content?

@smckisson

smckisson commented Dec 22, 2021

Copy link
Copy Markdown
Author

there's an error here, and the error got into the content, you need to delete the semicolon on line 374/368.

Can you also add a PR to content?

Fixed the compilation issue here and verified it compiles locally. I have prettier formatter in my VS Code env though so it reformatted the whole file. Feel free to reformat before merging with whatever formatter your team is using. The content PR doesn't have the semicolon before catch and looks correct to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants