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

[ABW-3869] Fix crash when transferring multiple NFTs from same collection #1366

Merged
merged 5 commits into from
Oct 7, 2024

Conversation

matiasbzurovski
Copy link
Contributor

@matiasbzurovski matiasbzurovski commented Oct 7, 2024

Jira ticket: ABW-3869

Description

This PR fixes a crash caused by attempting to add multiple values with same Key to a dictionary. The solution for this particular case required adding a new wrapper model to use IdentifiedArrayOf<> instead.
Still, the PR adds a custom throwing init for Dictionary and OrderedDictionary to at least catch these potential failures in other situations.

Video

Simulator.Screen.Recording.-.iPhone.16.-.2024-10-07.at.16.46.38.mp4

Comment on lines +7029 to +7030
shellScript = "# Custom linter to avoid using Dictionary.init(uniqueKeysWithValues:)\n# Quick solution before considering using an external tool, such as SwiftLint \n\nPATTERN=\"uniqueKeysWithValues:\"\nFILES=$(grep -rnw --include=\\*.swift \"$PATTERN\" \"$SRCROOT\")\n\nif [ ! -z \"$FILES\" ]; then\n echo \"Error: We shouldn't use Dictionary(uniqueKeysWithValues:), as it could lead to unhandled crashes if key is repeated\"\n echo \"Instead, use our custom init(keysWithValues:), or consider refactoring your solution to use IdentifiedArrayOf\"\n echo \"Please fix on the following files:\"\n echo \"$FILES\"\n exit 1 \nfi\n";
};
Copy link
Contributor Author

@matiasbzurovski matiasbzurovski Oct 7, 2024

Choose a reason for hiding this comment

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

Definitely not as great as SwiftLint, but we would need some time in order to integrate such tool (and define the rules that we want to include, asides this custom one). This simple script (that takes 0.1 seconds to run) will help prevent issues as this in the future.

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

nice, good job!

@matiasbzurovski matiasbzurovski merged commit 34b1fac into main Oct 7, 2024
6 checks passed
@matiasbzurovski matiasbzurovski deleted the ABW-3869-multiple-nft-crash branch October 7, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants