feat: Fixed unit tests and add tests for the new federation version #379
Conversation
| @@ -75,7 +75,7 @@ contract('Bridge Multichain Deploy Check', async function (accounts) { | |||
| const bridgeV4Implementation = new web3.eth.Contract(this.bridgeV4.abi, bridgeProxy.address); | |||
|
|
|||
| result = await bridgeV4Implementation.methods.version().call(); | |||
| assert.equal(result, 'v4'); | |||
| assert.equal(result, 'v3'); | |||
There was a problem hiding this comment.
I don't understand this change, if its v4 why is v3 expected?
There was a problem hiding this comment.
There is no V4 for bridge, this was wrong in the tests. I don't know if they planed to do it in the past but there is not Bridge V4
| logIndex | ||
| ); | ||
|
|
||
| require(votes[transactionId][_msgSender()] == false, "Federation: Federator already vote for this transaction"); |
There was a problem hiding this comment.
If we are using the require now, what is the purpose of keep returning a boolean? there are no cases of returning false, I would go full with require or full with boolean but mixing them is confusing
| {from: anAccount} | ||
| ), | ||
| truffleAssertions.ErrorType.REVERT | ||
| ); | ||
| }); | ||
|
|
||
| it('should fail if different array size', async function() { |
There was a problem hiding this comment.
can you explain why this test is removed? (just to have context)
There was a problem hiding this comment.
Because there is no array parameter in the contract more, there was in a old version.
| @@ -446,8 +408,14 @@ contract('FederationV2', async function (accounts) { | |||
| await this.allowTokens.setToken(originalTokenAddress, typeId); | |||
| this.sideTokenFactory = await SideTokenFactory.new(); | |||
| this.bridge = await Bridge.new(); | |||
| await this.bridge.methods['initialize(address,address,address,address)'](deployer, this.federators.address, | |||
| this.allowTokens.address, this.sideTokenFactory.address); | |||
| await this.bridge.methods['initialize(address,address,address,address,string)'] | |||
There was a problem hiding this comment.
Why this signature changes if the only contract in the PR with changes is federation? was this test outdated?
There was a problem hiding this comment.
Same as before, they updated the contract but didn't updated the tests.
| crossEvent.logIndex, | ||
| chains.ETHEREUM_MAIN_NET_CHAIN_ID, | ||
| chains.HARDHAT_TEST_NET_CHAIN_ID, | ||
| crossEvent.logIndex |
There was a problem hiding this comment.
this was outdated right? I don't see in FederationV2 the chain ids arguments in this event
There was a problem hiding this comment.
Most of my changes on the tests was to remove outdated values
| @@ -140,7 +140,7 @@ contract('Bridge upgrade test', async (accounts) => { | |||
| const newProxy = new web3.eth.Contract(BridgeV4.abi, this.proxyBridge.options.address); | |||
|
|
|||
| result = await newProxy.methods.version().call(); | |||
| assert.equal(result, 'v4'); | |||
| assert.equal(result, 'v3'); | |||
There was a problem hiding this comment.
all the fixes related to bridge functions signatures are because this tests were outdated?
|
|
||
| function changeRequirement(uint _required) external onlyOwner validRequirement(members.length, _required) | ||
| { | ||
| require(_required >= 2, "Federation: Requires at least 2"); |
There was a problem hiding this comment.
Can you please help to understand this?
|
Title of pull request
The pull request should be described here
Description
How to Test
ganache-cliCase 1
$~ cd bridge$~ npm run testExpected Result
$~ cd ../federatorExpected Result
Expected Result
Case N...
Checklist
Bridge Directory
cd bridgenpm run lintnpm run testnpm run compileFederator Directory
cd federatornpm run lintnpm run testnpm run build