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.

Sign up for GitHub

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

Jump to bottom

fix: generate salt inside macro call to avoid contract id collision #1177

Merged
merged 5 commits into from
Nov 1, 2023
Merged

fix: generate salt inside macro call to avoid contract id collision #1177

merged 5 commits into from
Nov 1, 2023

Conversation

iqdecay
Copy link
Contributor

@iqdecay iqdecay commented Oct 30, 2023

This PR fixes an issue where two contracts would have the same contract id because the salt was not generated properly.

digorithm
digorithm previously approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Excellent catch. I'm approving this, but we should have a test for this: a test that deploys the same contract twice and it checks for the different generated salts. This should prevent us from having this same incident in the future.

hal3e
hal3e requested changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

Great catch! Left some remarks...

packages/fuels-macros/src/setup_program_test/code_gen.rs Outdated Show resolved Hide resolved
packages/fuels-macros/src/setup_program_test/code_gen.rs Outdated Show resolved Hide resolved
@iqdecay iqdecay mentioned this pull request Oct 31, 2023
Open
@iqdecay
Copy link
Contributor Author

iqdecay commented Oct 31, 2023

Excellent catch. I'm approving this, but we should have a test for this: a test that deploys the same contract twice and it checks for the different generated salts. This should prevent us from having this same incident in the future.

#1181 tracking in this issue.

@iqdecay iqdecay requested review from digorithm and hal3e October 31, 2023 15:01
@iqdecay iqdecay merged commit a4d656f into master Nov 1, 2023
38 checks passed
@iqdecay iqdecay deleted the iqdecay/fix-salt-macros-recompile branch November 1, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@segfault-magnet segfault-magnet segfault-magnet approved these changes

@hal3e hal3e hal3e approved these changes

@Salka1988 Salka1988 Salka1988 approved these changes

@MujkicA MujkicA Awaiting requested review from MujkicA MujkicA is a code owner

@Br1ght0ne Br1ght0ne Awaiting requested review from Br1ght0ne

@digorithm digorithm Awaiting requested review from digorithm digorithm is a code owner

Assignees

@iqdecay iqdecay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@iqdecay @digorithm @Salka1988 @hal3e @segfault-magnet