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

Add xdai upgrade scripts for chai support #379

Merged
merged 4 commits into from Mar 13, 2020
Merged

Conversation

patitonar
Copy link
Contributor

Closes #377

This PR adds the scripts that can be used in the upgrade of xdai bridge to 4.0.0 release.

The following steps can be performed in order to upgrade the bridge:

  1. Deploy the new implementation contract for Foreign bridge.
  2. Disable transfers from home to foreign. Set HOME_DAILY_LIMIT=0 .env var and run setHomeDailyLimit script
  3. Wait until all in-progress transfers from home to foreign are processed.
  4. Upgrade oracles.
  5. Upgrade Foreign contract with the new implementation. Set NEW_IMPLEMENTATION_ETH_BRIDGE .env var and run chaiUpgrade:upgradeBridgeOnForeign script
  6. Initialize Chai and receiver. Set CHAI_INTEREST_RECEIVER .env var and run chaiUpgrade:initializeChai script
  7. Enable transfers from home to foreign again. Set HOME_DAILY_LIMIT >= 100000000000000000000001 (maxPerTx current value is '100000000000000000000000') .env var and run script: setHomeDailyLimit

Stopping the bridge operation from home to foreign is needed in order to ensure processing the transfers correctly since the way how the signatures are executed changes in this new release.

Testing the upgrade is still in-progress. I'll test the upgrade following the mentioned steps and add a comment with the results.

Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

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

I don't think it makes sense to keep old upgrade scripts. Let's substitute them by new ones. It will simplify the logic a bit.
If one wants to execute previous upgrade they could use the previous release tags/docker images. What do you think?

@akolotov
Copy link
Collaborator

Stopping the bridge operation from home to foreign is needed in order to ensure processing the transfers correctly since the way how the signatures are executed changes in this new release.

Instead of stopping the bridge by changing the limit (extra steps to be coordinated with validators) does it make sense to just roll back the block for watcher-erc-native-collected-signatures? The block must have the timestamp which is less or equal to the timestamp of the block where the foreign bridge implementation contract upgrade was confirmed.

@patitonar
Copy link
Contributor Author

I don't think it makes sense to keep old upgrade scripts. Let's substitute them by new ones. It will simplify the logic a bit.
If one wants to execute previous upgrade they could use the previous release tags/docker images. What do you think?

Yes, I think it is a good idea. I'll remove it

Instead of stopping the bridge by changing the limit (extra steps to be coordinated with validators) does it make sense to just roll back the block for watcher-erc-native-collected-signatures? The block must have the timestamp which is less or equal to the timestamp of the block where the foreign bridge implementation contract upgrade was confirmed.

Yes, we can avoid stopping the bridge. Actually, when processing the collected-signatures event with the old oracle version and if the new implementation contract is in place, when estimating the gas it will fail with Unknown error and it will continue re-trying listening to the same range of blocks. So when the new oracle version is used, it will correctly process the event. The same situation will be if first the oracle is updated and the new old implementation is still being used, it will fail at processing the event and will keep re-trying listening in that range of blocks until the contract is upgraded and the event is correctly processed. As you mention, in any case, we can rollback the watcher-erc-native-collected-signatures block number

@akolotov
Copy link
Collaborator

Great! As soon as you finish with preparing new implementation for testing the upgrade please let me know.

@akolotov akolotov changed the base branch from develop to master February 28, 2020 11:16
@patitonar
Copy link
Contributor Author

I pushed the changes mentioned. The mcdMigration scripts were removed and also the script to set the daily limit since it is not needed. Also cleaned up the .env.example to only include the variables needed for this upgrade.

The following steps can be performed to upgrade the bridge:

  1. Deploy the new implementation contract for Foreign bridge.
  2. Upgrade oracles. After this step Transfers from home to foreign won't be completed.
  3. Upgrade Foreign contract with the new implementation. Set NEW_IMPLEMENTATION_ETH_BRIDGE .env var and run upgradeBridgeOnForeign script. After this step pending transfers and new ones should be completed and work correctly.
  4. Initialize Chai and receiver. Set CHAI_INTEREST_RECEIVER .env var and run initializeChai script

I'll be testing this and add a comment with the results.

@patitonar
Copy link
Contributor Author

I tested the upgrade procedure in Sokol-Kovan and it worked correctly, no issues were found. However, I found one issue while testing the Chai integration features.

HomeBridge: 0xAEa775BC17BE99B2cd032CE2303b164BC2dC0dDD

ForeignBridge: 0xEF3D53Df20aCbB7b742C85d0F8Ca24F480AbE606

Multisig as foreign owner to test the upgrade scripts: 0x6829Df9a24D46f8C2fa22b9c59f9cf30FF49206a

Chai: 0x623f148dabd40dd221807c5e98235bc40e54a106 (Used the deployed Chai token mentioned here)

Here is the test performed:

  1. Started with the current implementation of contract and oracle
  2. Sent some transactions on both sides
  3. Deployed the new implementation contract in foreign.
  4. Upgrade oracle.
  5. Sent a transaction from Foreign to home and worked correctly. Sent a transaction from Home to Foreign and it failed at finishing processing it as expected because of how signatures are submitted.
  6. Upgrade Foreign contract with new implementation using the script upgradeBridgeOnForeign. First one validator as leader role and the other two as confirm. Here I set NEW_IMPLEMENTATION_ETH_BRIDGE=0x494e7C6679aE85DA86fBaC738ea76E3dC95411Df
  7. Checked that the pending transfer from Home to Foreign was completed correctly.
  8. Sent transactions in both directions.
  9. Initialized Chai and receiver using the script initializeChai. First one validator as leader role and the other two as confirm. Here I set CHAI_INTEREST_RECEIVER=0xf4BEF13F9f4f2B203FAF0C3cBbaAbe1afE056955.
  10. Sent some transactions on both sides. Dai tokens were converted to Chai.
  11. Waited some time to generate interest.
  12. Called payInterest() method. Here is the Tx.
  13. Tested running and processing transactions from an oracle instance deployed with the ansible deployment script.

In the step 11, when paying interests by calling payInterest() method, the generated interests in Dai are minted to the bridge contract and then the bridge transfers it to the InterestReceiver.
The Dai minted generated a Transfer event (from ZERO address to the bridge contract) that the bridge oracle transfer watcher tries to process to convert it to xdai. I think this is not the expected behavior. Instead, the bridge oracle should ignore this transfer, similar as it does for Transfer events generated by the token swap from Sai to Dai.

One possible fix for this, following how we handle these situations before, could be:

@k1rill-fedoseev
Copy link
Member

Is it necessary to emit a special event in such a case?
Seems like we can just skip Transfer events originated from address(0) in the oracle instead.

@akolotov
Copy link
Collaborator

akolotov commented Mar 9, 2020

The fix for found issue is being prepared here: #380

@akolotov akolotov merged commit a5946e7 into master Mar 13, 2020
@akolotov akolotov deleted the 377-chai-upgrade-script branch March 13, 2020 12:32
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.

Upgrade scripts to migrate xDai bridge to 4.0.0
3 participants