Skip to content

Fix: emit OwnershipTransferred after updating owner (closes #51)#53

Merged
mudgen merged 2 commits into
Perfect-Abstractions:mainfrom
CodeCryptX:fix/ownership-event-order
Oct 21, 2025
Merged

Fix: emit OwnershipTransferred after updating owner (closes #51)#53
mudgen merged 2 commits into
Perfect-Abstractions:mainfrom
CodeCryptX:fix/ownership-event-order

Conversation

@CodeCryptX

Copy link
Copy Markdown
Contributor

Summary

Fixed the event emission order in ERC173 facet and library.

Details

  • Moved OwnershipTransferred event emission after updating owner.
  • Improves clarity and aligns with Solidity event emission best practices.

Closes #51

@adamgall adamgall left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this is a good change. It introduces extra variables (gas costs) with no change in functionality.

@maxnorm

maxnorm commented Oct 20, 2025

Copy link
Copy Markdown
Collaborator

You're not wrong for the extra gaz costs.

Can storage update fail for some reasons? If not, i agree that the change is not needed

@mudgen mudgen merged commit 7e764e8 into Perfect-Abstractions:main Oct 21, 2025
JackieXu pushed a commit to JackieXu/Compose that referenced this pull request Nov 6, 2025
…hip-event-order

Fix: emit OwnershipTransferred after updating owner (closes Perfect-Abstractions#51)
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.

[style] ERC173: emit OwnershipTransferred after updating owner state

4 participants