-
Notifications
You must be signed in to change notification settings - Fork 26
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
Merged minter and mass conservation ERC 20 bridges into one bridge #35
Conversation
…ntract Signed-off-by: Peter Robinson <drinkcoffee@eml.cc>
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. Added some minor comments
* @param _recipient Account to transfer ownership of the tokens to. | ||
* @param _amount The number of tokens to be transferred. | ||
*/ | ||
function receiveFromOtherBlockchain(address _destTokenContract, address _recipient, uint256 _amount) whenNotPaused external { |
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.
this is an extreme edge-case I imagine, but I wonder in a scenario where we have to pause the bridge on both ends, what might happen to events in flight (i.e. still gathering attestations or waiting on finalisation window). Is there a chance that these events would be permanently lost if the bridge is paused longer than the timehorizon for events?
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.
There are multiple levels of pausing for the ERC 20 bridge. The ERC 20 contacts can pause. The ERC 20 bridge can pause. The Simple Function Call contract can pause. The SFC and ERC 20 bridge pause on the sending side only. However, the ERC 20 contract on the destination could be paused. In that case, an in-flight transaction would fail. The only way to resolve this would be to call "adminTransfer" on the source blockchain to effect a refund.
* @param _thisBcTokenContract Address of ERC 20 contract on this blockchain. | ||
* @param _thisBcMassC True if the token contract on this blockchain uses mass conservation. | ||
*/ | ||
function setTokenConfig(address _thisBcTokenContract, bool _thisBcMassC) external { |
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.
This function feels a bit dangerous given it can potentially lead to inconsistencies if used after a transfer has occurred a) is it critical to have? could the solution to an initial misconfiguration just be to redeploy and remap a new contract? b) if we should keep it, should we do some check to see if a transfer using the existing mechanism has happened and if so, reject the change? I imagine this adds a fair complexity c) perhaps emitting an event if this is called might be useful?
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.
There is one bridge, and many token contracts. The token contract could have already been deployed, and not be under the control of the bridge operators. As such, its address is fixed. The bridge can't be redeployed because it has multiple token contracts, and the address of the bridge is configured in remote bridge contracrts.
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.
As such, a) is not an option.
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.
For Simple Function Call protocol, once a transaction on a source blockchain or a destination blockchain completes, then it is finished. As such, b) isn't a problem.
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.
c) is correct. An event should be emitted. I will add this and push the 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.
a) right, I was referring to redeployment of the token contract and not the bridge but I see how this could still be difficult.
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 pushed changes to add the event: c01a12f
I also added in some tests.
…rect events are being emitted Signed-off-by: Peter Robinson <drinkcoffee@eml.cc>
This PR resolves: #34
The abstract, minter-burner, mass conservation, and interface for the SFC ERC 20 bridge have been combined into one contract. The new ERC 20 bridge requires that when new ERC 20 tokens are added to the contract, that the type of contract on the blockchain is specified (mint & burn or mass conservation). At the same time as the ERC 20 token is added for the first time, the linkage between the token contract on this blockchain and on remote blockchains is set-up.
The type of contract can be updated. This would only be done if the original setting was incorrect.
Additional ERC 20 contracts on other blockchain linkages can be added.
Signed-off-by: Peter Robinson drinkcoffee@eml.cc