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

suggestions for COA address #5344

Merged
merged 6 commits into from
Feb 2, 2024
Merged

suggestions for COA address #5344

merged 6 commits into from
Feb 2, 2024

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Feb 1, 2024 •
edited by j1010001
Loading

closes #5179

  • add comments to clarify:
    • security reasoning behind choosing an address prefix of 12 bytes
    • reasoning behind the index to postfix mapping
  • rename some function and constants to match the underlying logic
  • minor code simplification

turbolent
turbolent reviewed Feb 1, 2024
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

fvm/evm/handler/addressAllocator.go
// into deterministic random-looking address postfixes.
// The constant must be an ODD number. It is a "nothing-up-my-sleeves" constant.
// Look at `mapAddressIndex` for more details.
addressIndexMultiplierConstant = uint64(0xFFEEDDCCBBAA9977)
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning for the change from 0xFFEEDDCCBBAA9987 to 0xFFEEDDCCBBAA9977?

Copy link
Contributor Author

@tarakby tarakby Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I subjectively thought it looks more like a "nothing up my sleeve" constant this way (we couldn't use 88 because it's even so we jumped to 77). 87 is fine too.

Copy link
Member

@turbolent turbolent Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I see now 👍 Maybe add the part "we couldn't use 88 because it's even so we jumped to 77"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4fa6fda

@tarakby tarakby mentioned this pull request Feb 1, 2024
Merged
@ramtinms ramtinms merged commit 5c410f9 into ramtin/5226-coa-smart-contracts Feb 2, 2024
@ramtinms ramtinms deleted the tarak/address-suggestions branch February 2, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbolent turbolent turbolent left review comments

@ramtinms ramtinms ramtinms approved these changes

@janezpodhostnik janezpodhostnik Awaiting requested review from janezpodhostnik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tarakby @turbolent @ramtinms