-
Notifications
You must be signed in to change notification settings - Fork 28
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
Refactor token source contracts #229
Conversation
Refactor token source contracts
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, just a few small comments.
contracts/src/CrossChainApplications/examples/NativeTokenBridge/ITokenSource.sol
Outdated
Show resolved
Hide resolved
*/ | ||
function _unlockTokens(address recipient, uint256 amount) private { | ||
function _unlockTokens(address recipient, uint256 amount) internal override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling out that the visibility of _unlockTokens
and _handleBurnTokens
is changing from private
to internal
. This should be okay, given that we don't expect anything to inherit from these contracts (and if someone does, that's on them), but it is a minor change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -0,0 +1,96 @@ | |||
// (c) 2024, Ava Labs, Inc. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should update the other licenses to 2024 before launch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we should update any old copyright dates. This SO thread aligns with my understanding of copyright law https://stackoverflow.com/questions/2390230/do-copyright-dates-need-to-be-updated
contracts/src/CrossChainApplications/examples/NativeTokenBridge/TokenSource.sol
Show resolved
Hide resolved
contracts/src/CrossChainApplications/examples/NativeTokenBridge/TokenSource.sol
Outdated
Show resolved
Hide resolved
contracts/src/CrossChainApplications/examples/NativeTokenBridge/TokenSource.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some nits
ffe08c3
…e/TokenSource.sol Co-authored-by: minghinmatthewlam <matthew.lam@avalabs.org> Signed-off-by: Geoff Stuart <geoff.vball@gmail.com>
contracts/src/CrossChainApplications/examples/NativeTokenBridge/NativeTokenDestination.sol
Outdated
Show resolved
Hide resolved
…e/NativeTokenDestination.sol Co-authored-by: minghinmatthewlam <matthew.lam@avalabs.org> Signed-off-by: Geoff Stuart <geoff.vball@gmail.com>
f6fd719
67d1279
Why this should be merged
Closes #219
How this works
Extracts common code from
ERC20TokenSource
andNativeTokenSource
into an abstract contractTokenSource
.How this was tested
Existing tests
How is this documented
N/A