Unit test cases for progressStakeWithProof #578
Conversation
| gasLimit: new BN(stakeParams.gasLimit, 16), | ||
| nonce: new BN(stakeParams.nonce, 16), | ||
| hashLock: stakeParams.hashLock, | ||
| messageHash: proofData.gateway.stake.return_value.returned_value.messageHash_, |
There was a problem hiding this comment.
Yes, any suggestion?
I can store the values in some variable in before each.
There was a problem hiding this comment.
That would just move the deepness somewhere else. I don't know how to solve this, I don't know the scope of proof data.
contracts/gateway/EIP20Gateway.sol
Outdated
| redeemer_ = message.sender; | ||
| redeemerNonce_ = message.nonce; | ||
| amount_ = unstakes[_messageHash].amount; | ||
|
|
||
| // delete the unstake data | ||
| delete unstakes[_messageHash]; |
There was a problem hiding this comment.
General question, we delete stakes/unstakes mapping in progress/progressRevert, but not message mapping. Messages are deleted when new messages are created. why?
There was a problem hiding this comment.
After progress and revert you may not be able to do anything with stakes/unstakes mapping else so its deleted. Message state is not deleted because it may be used for merkle proof.
But in fact, I am thinking not to delete the message state.
- Stake
- ConfirmStake
- ProgressMintWithProof. (deletes the mints stuct data, the proof was not generated)
- redeem (deletes the message status)
- ProgressStakeWithProof (Now the proof will not be generated.)
There was a problem hiding this comment.
I understand we can't clean message status mapping and it has valid reason.
My point was for message mapping. Messages can be deleted with stake, mint, redeem and unstakes mapping.
If you and @schemar agree on this, seperate ticket can be created for this.
|
|
||
| }); | ||
|
|
||
| it('should fail when storage proof is invalid', async function () { |
There was a problem hiding this comment.
I dont think this test is needed. This test is asserting failure of RLP library which is out of scope of this test.
There was a problem hiding this comment.
As progressStakeWithProof is not an ideal unit test. It has integration with messagebus library and gatewayLib, I am considering all this as one unit. I would prefer to keep this.
This will be coved in #526 |
This PR includes the unit test for
EIP20Gateway.progressStakeWithProof.The proof data is added.
fixes: #424