Add tests for Celo2 Signer compatibility with both Celo1 and Celo2 transactions #308
Conversation
|
|
||
| // Test_forks_findTxFuncs tests that the correct txFuncs are returned for a given transaction type and chain config | ||
| func Test_forks_findTxFuncs(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Hey @Kourin1996 I think this test would be more useful if it tested from the level of the Signer using a similar approach to TestProtectedCeloLegacyTxSigning.
So generating a Signer for a given config and then signing each transaction with that signer and checking that the sender address can be correctly recovered, of course for some transactions an error will be returned because they are not supported or deprecated.
There was a problem hiding this comment.
I believe such cases are covered by TestCeloSigner_SignAndRecovery. TestCeloSigner_SignAndRecovery test focuses on signing only in Celo2 but includes actual scenarios.
On the other hand, this Test_forks_findTxFuncs test is just a low-level test designed to detect unexpected changes.
There was a problem hiding this comment.
Good point, what's being tested here is already covered by the two tests you mentioned. It seems to me that this test doesn’t add much value, so my preference would be to remove it. That said, I’m also fine with keeping it if you feel strongly about it.
Closes #168
This PR introduces unit tests for
celoSignerwhich was added in #165The tests verify the following things:
celoSignerrecover the sender's address from transactions created on Celo1 blockchainceloSignersigns transactions for supported transaction types in Celo2celoSignercorrectly rejects signing unsupported transaction types in Celo2celoSigneris designed to support transaction types compatible with Celo1 for history data support. Therefore, this PR includes signatures generated using celo-blockchain version v1.8.7. (Note: v1.8.4 produces identical results)