Refactor BurnMintERC677 to inherit from generic BurnMintERC20 #14605
Conversation
jhweintraub
requested review from
RensR,
matYang,
RayXpub and
elatoskinas
as code owners
Static analysis results are availableHey @jhweintraub, you can view Slither reports in the job summary here or download them as artifact here. Please check them before merging and make sure you have addressed all issues. |
app-token-issuer-infra-releng
bot
requested a review
from a team
as a code owner
app-token-issuer-infra-releng
bot
requested review from
george-dorin
and removed request for
a team
Solidity Review Jira issueHey! We have taken the liberty to link this PR to a Jira issue for Solidity Review. This is a new feature, that's currently in the pilot phase, so please make sure that the linkage is correct. In a contrary case, please update it manually in JIRA and replace Solidity Review issue key in the changeset file with the correct one. Any changes to the Solidity Review Jira issue should be reflected in the changeset file. If you need to update the issue key, please do so manually in the following changeset file: |
…-Generic-BurnMintERC20
let's see if removing this additional extra changeset file fixes anything
try moving the changeset tag to the beginning?
…-Generic-BurnMintERC20
…-Generic-BurnMintERC20
…-Generic-BurnMintERC20
…-Generic-BurnMintERC20
…-Generic-BurnMintERC20
…-Generic-BurnMintERC20
…-Generic-BurnMintERC20
…-Generic-BurnMintERC20
|
|
||
| contract BurnMintERC677Helper is BurnMintERC677, IGetCCIPAdmin { | ||
| contract BurnMintERC677Helper is BurnMintERC677 { |
There was a problem hiding this comment.
I probably removed it somewhere in the development process when it was causing inheritance issues and forgot to put it back in at some point. This is a helper contract does it matter?
There was a problem hiding this comment.
Why would you remove it if it doesn't matter? This is deployed to testnets so yes it matters.
There was a problem hiding this comment.
I mean I had the opposite belief, that if it didn't matter then remove it for readability but thats fine it's been re-added anyways, this comment is stale anyways
| @@ -1932,11 +1932,11 @@ func (LinkTokenOwnershipTransferred) Topic() common.Hash { | |||
| } | |||
|
|
|||
| func (LinkTokenTransfer) Topic() common.Hash { | |||
| return common.HexToHash("0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef") | |||
| return common.HexToHash("0xe19260aff97b920c7df27010903aeb9c8d2be5d310a2c67824cf3f15396e4c16") | |||
There was a problem hiding this comment.
What causes this diff? Can we undo that?
There was a problem hiding this comment.
auto generated by the CI gethwrappers
There was a problem hiding this comment.
I know that, but I'd like it to stay the same. Why would the hash change was my question, did something in this onchain logic change?
There was a problem hiding this comment.
It shouldn't have. I will investigate.
jhweintraub
dismissed stale reviews from 0xsuryansh, RyanRHall, and matYang
via
88b81bb
…-Generic-BurnMintERC20
…-Generic-BurnMintERC20
|
Pull request was closed
The existing implementation of BurnMintERC677 inherits the ERC20 functionality from OZ, but not any of the BurnMint Functionality that is desired from it. As a result there exists no standard BurnMintERC20 implementation for Chainlink projects/customers to use without requiring they also support ERC-677 functionality
This PR restructures the
BurnMintERC677implementation to inherit from a more genericBurnMintERC20and then adds thetransferAndCallmethod and updates tosupportsInterface()in to ensure compliance with ERC677. The existing base implementation of the standardERC677.solhas not been modified.