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

Enhance documentation in TokenCallbackHandler with ERC-1820 registration details #885

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Jan 6, 2025

This PR adds a detailed comment in the TokenCallbackHandler contract, clarifying the requirement for accounts to register the implementer via the ERC-1820 interface registry to receive ERC777 tokens. This update aims to improve clarity and understanding of the token reception process.

No functional changes were made to the contract logic.

@mmv08 mmv08 requested review from a team, rmeissner, nlordell, akshay-ap and remedcu and removed request for a team January 6, 2025 15:38
Copy link
Member

@remedcu remedcu left a comment

Choose a reason for hiding this comment

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

nit: unrelated to this PR.

* @dev Account that wishes to receive the tokens also needs to register the implementer (this contract) via the ERC-1820 interface registry.
* From the standard: "This is done by calling the setInterfaceImplementer function on the ERC-1820 registry with the holder address as
* the address, the keccak256 hash of ERC777TokensSender (0x29ddb589b1fb5fc7cf394961c1adf5f8c6454761adf795e67fe149f658abe895) as the
* interface hash, and the address of the contract implementing the ERC777TokensSender as the implementer."
* return nothing (not standardized)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* return nothing (not standardized)
* @return nothing (not standardized)

Copy link
Member

Choose a reason for hiding this comment

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

My bad, maybe add it as a sentence, as this one seems like the end of the previous line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it because it adds no value. We also don't specify empty returns in other contracts' NatSpec.

…ion details

This commit adds a detailed comment in the TokenCallbackHandler contract, clarifying the requirement for accounts to register the implementer via the ERC-1820 interface registry to receive ERC777 tokens. This update aims to improve clarity and understanding of the token reception process.

No functional changes were made to the contract logic.
@mmv08 mmv08 force-pushed the erc777-compat-comments branch from bdf63fa to 137558b Compare January 9, 2025 15:39
@mmv08 mmv08 merged commit c92ddef into main Jan 9, 2025
25 checks passed
@mmv08 mmv08 deleted the erc777-compat-comments branch January 9, 2025 15:57
@github-actions github-actions bot locked and limited conversation to collaborators Jan 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants