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 tests for Celo2 Signer compatibility with both Celo1 and Celo2 transactions #308

Merged
merged 7 commits into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/types/celo_transaction_signing_forks.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,11 @@ func (c *celoLegacy) txFuncs(tx *Transaction) *txFuncs {
return celoLegacyTxFuncs
case t == DynamicFeeTxType:
// We handle the dynamic fee tx type here because we need to handle
// migrated dynamic fee txs. These were enabeled in celo in the Espresso
// migrated dynamic fee txs. These were enabled in celo in the Espresso
// hardfork, which doesn't have any analogue in op-geth. Even though
// op-geth does enable support for dynamic fee txs in the London
// hardfork (which we set to the cel2 block) that fork contains a lot of
// changes that were not part of Espresso. So instead we ned to handle
// changes that were not part of Espresso. So instead we need to handle
// DynamicFeeTxTypes here.
return dynamicFeeTxFuncs
case t == AccessListTxType:
Expand Down
100 changes: 100 additions & 0 deletions core/types/celo_transaction_signing_forks_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
package types

import (
"testing"

"github.com/ethereum/go-ethereum/params"
"github.com/stretchr/testify/assert"
)

// Test_forks_activeForks tests that the correct forks are returned for a given block time and chain config
func Test_forks_activeForks(t *testing.T) {
t.Parallel()

cel2Time := uint64(1000)

t.Run("Non-Celo", func(t *testing.T) {
config := &params.ChainConfig{
Cel2Time: nil,
}
assert.Equal(t, []fork(nil), celoForks.activeForks(1000, config))
})

t.Run("Celo1", func(t *testing.T) {
config := &params.ChainConfig{
Cel2Time: &cel2Time,
}
assert.Equal(t, []fork{&celoLegacy{}}, celoForks.activeForks(500, config))
})

t.Run("Celo2", func(t *testing.T) {
config := &params.ChainConfig{
Cel2Time: &cel2Time,
}
assert.Equal(t, []fork{&cel2{}, &celoLegacy{}}, celoForks.activeForks(1000, config))
})
}

// 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()
Copy link

@piersy piersy Jan 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@piersy

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.

Copy link

@piersy piersy Jan 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@piersy OK. I removed it!


cel2Time := uint64(1000)
config := &params.ChainConfig{
Cel2Time: &cel2Time,
}

celo2Forks := forks(celoForks.activeForks(cel2Time, config))
t.Run("Celo LegacyTx in Celo2", func(t *testing.T) {
assert.Equal(t, deprecatedTxFuncs, celo2Forks.findTxFuncs(NewTx(&LegacyTx{CeloLegacy: true})))
})

t.Run("Ethereum LegacyTx in Celo2", func(t *testing.T) {
assert.Equal(t, (*txFuncs)(nil), celo2Forks.findTxFuncs(NewTx(&LegacyTx{CeloLegacy: false})))
})

t.Run("AccessListTx in Celo2", func(t *testing.T) {
assert.Equal(t, accessListTxFuncs, celo2Forks.findTxFuncs(NewTx(&AccessListTx{})))
})

t.Run("DynamicFeeTx in Celo2", func(t *testing.T) {
assert.Equal(t, dynamicFeeTxFuncs, celo2Forks.findTxFuncs(NewTx(&DynamicFeeTx{})))
})

t.Run("CeloDynamicFeeTx in Celo2", func(t *testing.T) {
assert.Equal(t, deprecatedTxFuncs, celo2Forks.findTxFuncs(NewTx(&CeloDynamicFeeTx{})))
})

t.Run("CeloDynamicFeeTxV2 in Celo2", func(t *testing.T) {
assert.Equal(t, celoDynamicFeeTxV2Funcs, celo2Forks.findTxFuncs(NewTx(&CeloDynamicFeeTxV2{})))
})

celo1Forks := forks(celoForks.activeForks(uint64(100), config))
t.Run("Celo LegacyTx in Celo1", func(t *testing.T) {
assert.Equal(t, celoLegacyTxFuncs, celo1Forks.findTxFuncs(NewTx(&LegacyTx{CeloLegacy: true})))
})

t.Run("Ethereum LegacyTx in Celo1", func(t *testing.T) {
assert.Equal(t, (*txFuncs)(nil), celo1Forks.findTxFuncs(NewTx(&LegacyTx{CeloLegacy: false})))
})

t.Run("AccessListTx in Celo1", func(t *testing.T) {
assert.Equal(t, accessListTxFuncs, celo1Forks.findTxFuncs(NewTx(&AccessListTx{})))
})

t.Run("DynamicFeeTx in Celo1", func(t *testing.T) {
assert.Equal(t, dynamicFeeTxFuncs, celo1Forks.findTxFuncs(NewTx(&DynamicFeeTx{})))
})

t.Run("CeloDynamicFeeTx in Celo1", func(t *testing.T) {
assert.Equal(t, celoDynamicFeeTxFuncs, celo1Forks.findTxFuncs(NewTx(&CeloDynamicFeeTx{})))
})

t.Run("CeloDynamicFeeTxV2 in Celo1", func(t *testing.T) {
assert.Equal(t, celoDynamicFeeTxV2Funcs, celo1Forks.findTxFuncs(NewTx(&CeloDynamicFeeTxV2{})))
})

t.Run("CeloDenominatedTx in Celo1", func(t *testing.T) {
assert.Equal(t, (*txFuncs)(nil), celo1Forks.findTxFuncs(NewTx(&CeloDenominatedTx{})))
})
}
Loading
Loading