Skip to content

chore: quick cleanup #2239

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.

Sign up for GitHub

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

Merged
tac0turtle merged 4 commits into from
May 2, 2025
Merged

chore: quick cleanup #2239

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ab3d7a
quick cleanup
May 2, 2025
eb735e9
Merge branch 'main' into marko/cleanup
May 2, 2025
c07c920
Update yaml.go
tac0turtle May 2, 2025
aaf0a3f
revert some
May 2, 2025
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
28 changes: 13 additions & 15 deletions block/publish_block_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,6 @@ func Test_publishBlock_EmptyBatch(t *testing.T) {
// is successfully created, applied, and published.
func Test_publishBlock_Success(t *testing.T) {
require := require.New(t)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

initialHeight := uint64(5)
newHeight := initialHeight + 1
Expand All @@ -311,25 +309,25 @@ func Test_publishBlock_Success(t *testing.T) {
manager, mockStore, mockExec, mockSeq, _, headerCh, dataCh, _ := setupManagerForPublishBlockTest(t, true, initialHeight, 0)
manager.lastState.LastBlockHeight = initialHeight

mockStore.On("Height", ctx).Return(initialHeight, nil).Once()
mockStore.On("Height", t.Context()).Return(initialHeight, nil).Once()
mockSignature := types.Signature([]byte{1, 2, 3})
mockStore.On("GetSignature", ctx, initialHeight).Return(&mockSignature, nil).Once()
mockStore.On("GetSignature", t.Context(), initialHeight).Return(&mockSignature, nil).Once()
lastHeader, lastData := types.GetRandomBlock(initialHeight, 5, chainID)
lastHeader.ProposerAddress = manager.genesis.ProposerAddress
mockStore.On("GetBlockData", ctx, initialHeight).Return(lastHeader, lastData, nil).Once()
mockStore.On("GetBlockData", ctx, newHeight).Return(nil, nil, errors.New("not found")).Once()
mockStore.On("SaveBlockData", ctx, mock.AnythingOfType("*types.SignedHeader"), mock.AnythingOfType("*types.Data"), mock.AnythingOfType("*types.Signature")).Return(nil).Once()
mockStore.On("SaveBlockData", ctx, mock.AnythingOfType("*types.SignedHeader"), mock.AnythingOfType("*types.Data"), mock.AnythingOfType("*types.Signature")).Return(nil).Once()
mockStore.On("SetHeight", ctx, newHeight).Return(nil).Once()
mockStore.On("UpdateState", ctx, mock.AnythingOfType("types.State")).Return(nil).Once()
mockStore.On("SetMetadata", ctx, LastBatchDataKey, mock.AnythingOfType("[]uint8")).Return(nil).Once()
mockStore.On("GetBlockData", t.Context(), initialHeight).Return(lastHeader, lastData, nil).Once()
mockStore.On("GetBlockData", t.Context(), newHeight).Return(nil, nil, errors.New("not found")).Once()
mockStore.On("SaveBlockData", t.Context(), mock.AnythingOfType("*types.SignedHeader"), mock.AnythingOfType("*types.Data"), mock.AnythingOfType("*types.Signature")).Return(nil).Once()
mockStore.On("SaveBlockData", t.Context(), mock.AnythingOfType("*types.SignedHeader"), mock.AnythingOfType("*types.Data"), mock.AnythingOfType("*types.Signature")).Return(nil).Once()
mockStore.On("SetHeight", t.Context(), newHeight).Return(nil).Once()
mockStore.On("UpdateState", t.Context(), mock.AnythingOfType("types.State")).Return(nil).Once()
mockStore.On("SetMetadata", t.Context(), LastBatchDataKey, mock.AnythingOfType("[]uint8")).Return(nil).Once()

// --- Mock Executor ---
sampleTxs := [][]byte{[]byte("tx1"), []byte("tx2")}
// No longer mocking GetTxs since it's handled by reaper.go
newAppHash := []byte("newAppHash")
mockExec.On("ExecuteTxs", ctx, mock.Anything, newHeight, mock.AnythingOfType("time.Time"), manager.lastState.AppHash).Return(newAppHash, uint64(100), nil).Once()
mockExec.On("SetFinal", ctx, newHeight).Return(nil).Once()
mockExec.On("ExecuteTxs", t.Context(), mock.Anything, newHeight, mock.AnythingOfType("time.Time"), manager.lastState.AppHash).Return(newAppHash, uint64(100), nil).Once()
mockExec.On("SetFinal", t.Context(), newHeight).Return(nil).Once()

// No longer mocking SubmitRollupBatchTxs since it's handled by reaper.go
batchTimestamp := lastHeader.Time().Add(1 * time.Second)
Expand All @@ -344,8 +342,8 @@ func Test_publishBlock_Success(t *testing.T) {
batchReqMatcher := mock.MatchedBy(func(req coresequencer.GetNextBatchRequest) bool {
return string(req.RollupId) == chainID
})
mockSeq.On("GetNextBatch", ctx, batchReqMatcher).Return(batchResponse, nil).Once()
err := manager.publishBlock(ctx)
mockSeq.On("GetNextBatch", t.Context(), batchReqMatcher).Return(batchResponse, nil).Once()
err := manager.publishBlock(t.Context())
require.NoError(err, "publishBlock should succeed")

select {
Expand Down
17 changes: 5 additions & 12 deletions block/reaper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package block

import (
"context"
"crypto/sha256"
"encoding/hex"
"testing"
Expand All @@ -18,8 +17,6 @@ import (
)

func TestReaper_SubmitTxs_Success(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

mockExec := testmocks.NewExecutor(t)
mockSeq := testmocks.NewSequencer(t)
Expand All @@ -28,7 +25,7 @@ func TestReaper_SubmitTxs_Success(t *testing.T) {
chainID := "test-chain"
interval := 100 * time.Millisecond

reaper := NewReaper(ctx, mockExec, mockSeq, chainID, interval, logger, store)
reaper := NewReaper(t.Context(), mockExec, mockSeq, chainID, interval, logger, store)

// Prepare transaction and its hash
tx := []byte("tx1")
Expand All @@ -55,8 +52,6 @@ func TestReaper_SubmitTxs_Success(t *testing.T) {
}

func TestReaper_SubmitTxs_NoTxs(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

mockExec := testmocks.NewExecutor(t)
mockSeq := testmocks.NewSequencer(t)
Expand All @@ -65,7 +60,7 @@ func TestReaper_SubmitTxs_NoTxs(t *testing.T) {
chainID := "test-chain"
interval := 100 * time.Millisecond

reaper := NewReaper(ctx, mockExec, mockSeq, chainID, interval, logger, store)
reaper := NewReaper(t.Context(), mockExec, mockSeq, chainID, interval, logger, store)

// Mock GetTxs returning no transactions
mockExec.On("GetTxs", mock.Anything).Return([][]byte{}, nil).Once()
Expand All @@ -80,8 +75,6 @@ func TestReaper_SubmitTxs_NoTxs(t *testing.T) {

func TestReaper_TxPersistence_AcrossRestarts(t *testing.T) {
require := require.New(t)
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

// Use separate mocks for each instance but share the store
mockExec1 := testmocks.NewExecutor(t)
Expand All @@ -100,7 +93,7 @@ func TestReaper_TxPersistence_AcrossRestarts(t *testing.T) {
txKey := ds.NewKey(hex.EncodeToString(txHash[:]))

// First reaper instance
reaper1 := NewReaper(ctx, mockExec1, mockSeq1, chainID, interval, logger, store)
reaper1 := NewReaper(t.Context(), mockExec1, mockSeq1, chainID, interval, logger, store)

// Mock interactions for the first instance
mockExec1.On("GetTxs", mock.Anything).Return([][]byte{tx}, nil).Once()
Expand All @@ -112,12 +105,12 @@ func TestReaper_TxPersistence_AcrossRestarts(t *testing.T) {
reaper1.SubmitTxs()

// Verify the tx was marked as seen in the real store after the first run
has, err := store.Has(ctx, txKey)
has, err := store.Has(t.Context(), txKey)
require.NoError(err)
require.True(has, "Transaction should be marked as seen in the datastore after first submission")

// Create a new reaper instance simulating a restart
reaper2 := NewReaper(ctx, mockExec2, mockSeq2, chainID, interval, logger, store)
reaper2 := NewReaper(t.Context(), mockExec2, mockSeq2, chainID, interval, logger, store)

// Mock interactions for the second instance
mockExec2.On("GetTxs", mock.Anything).Return([][]byte{tx}, nil).Once()
Expand Down
2 changes: 1 addition & 1 deletion block/retriever.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (m *Manager) processNextDAHeader(ctx context.Context) error {

var err error
m.logger.Debug("trying to retrieve block from DA", "daHeight", daHeight)
for r := 0; r < dAFetcherRetries; r++ {
for range dAFetcherRetries {

Copilot AI May 2, 2025

Copy link

Choose a reason for hiding this comment

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

Using 'for range' on 'dAFetcherRetries' (an integer) is not valid. Consider using 'for r := 0; r < dAFetcherRetries; r++' instead.

Suggested change
for range dAFetcherRetries {
for r := 0; r < dAFetcherRetries; r++ {

Copilot uses AI. Check for mistakes.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
select {
case <-ctx.Done():
return ctx.Err()
Expand Down
4 changes: 1 addition & 3 deletions block/submitter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,7 @@ daSubmitRetryLoop:
maxBlobSize = initialMaxBlobSize
if m.gasMultiplier > 0 && gasPrice != -1 {
gasPrice = gasPrice / m.gasMultiplier
if gasPrice < initialGasPrice {
gasPrice = initialGasPrice
}
gasPrice = max(gasPrice, initialGasPrice)
Comment thread
tac0turtle marked this conversation as resolved.
}
m.logger.Debug("resetting DA layer submission options", "backoff", backoff, "gasPrice", gasPrice, "maxBlobSize", maxBlobSize)
case coreda.StatusNotIncludedInBlock, coreda.StatusAlreadyInMempool:
Expand Down
8 changes: 4 additions & 4 deletions node/full_node_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func (s *FullNodeTestSuite) SetupTest() {
s.T().Logf("DA client initialized: %v", s.node.blockManager.DALCInitialized())

// Wait longer for height to stabilize and log intermediate values
for i := 0; i < 5; i++ {
for range 5 {
time.Sleep(200 * time.Millisecond)
currentHeight, err := s.node.Store.Height(s.ctx)
require.NoError(err)
Expand Down Expand Up @@ -236,7 +236,7 @@ func (s *FullNodeTestSuite) TestSubmitBlocksToDA() {

// Monitor batch retrieval
s.T().Log("=== Monitoring Batch Retrieval ===")
for i := 0; i < 5; i++ {
for i := range 5 {
time.Sleep(200 * time.Millisecond)
// We can't directly check batch queue size, but we can monitor block production
currentHeight, err := s.node.Store.Height(s.ctx)
Expand All @@ -252,7 +252,7 @@ func (s *FullNodeTestSuite) TestSubmitBlocksToDA() {
s.node.blockManager.SetLastState(currentState)

// Monitor after trigger
for i := 0; i < 5; i++ {
for i := range 5 {
time.Sleep(200 * time.Millisecond)
currentHeight, err := s.node.Store.Height(s.ctx)
require.NoError(err)
Expand Down Expand Up @@ -288,7 +288,7 @@ func (s *FullNodeTestSuite) TestDAInclusion() {

// Monitor state changes in shorter intervals
s.T().Log("=== Monitoring State Changes ===")
for i := 0; i < 10; i++ {
for i := range 10 {
time.Sleep(200 * time.Millisecond)
currentHeight, err := s.node.Store.Height(s.ctx)
require.NoError(err)
Expand Down
4 changes: 2 additions & 2 deletions node/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ func (m MockTester) Fail() {}
func (m MockTester) FailNow() {}

// Logf is used to log a message to the test logger
func (m MockTester) Logf(format string, args ...interface{}) {}
func (m MockTester) Logf(format string, args ...any) {}

// Errorf is used to log an error to the test logger
func (m MockTester) Errorf(format string, args ...interface{}) {}
func (m MockTester) Errorf(format string, args ...any) {}

func waitForFirstBlock(node Node, source Source) error {
return waitForAtLeastNBlocks(node, 1, source)
Expand Down
4 changes: 2 additions & 2 deletions pkg/cache/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ func TestCacheConcurrency(t *testing.T) {
var wg sync.WaitGroup
wg.Add(goroutines)

for i := 0; i < goroutines; i++ {
for i := range goroutines {
go func(id int) {
defer wg.Done()
for j := 0; j < operations; j++ {
for j := range operations {

Copilot AI May 2, 2025

Copy link

Choose a reason for hiding this comment

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

The variable 'goroutines' is an integer and cannot be directly iterated over using 'for range'. Consider replacing with 'for i := 0; i < goroutines; i++'.

Copilot uses AI. Check for mistakes.

Copilot AI May 2, 2025

Copy link

Choose a reason for hiding this comment

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

Since 'operations' is an integer, iterating over it with 'for range' is invalid. Use a traditional loop syntax like 'for j := 0; j < operations; j++' instead.

Copilot uses AI. Check for mistakes.
// Test concurrent item operations
Comment on lines +102 to 106

@coderabbitai coderabbitai Bot May 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Range-over-integer causes compilation failure

range expects an iterable (array, slice, map, string, channel).
Here both goroutines and operations are plain int constants, so the test will not compile (cannot range over <int>).

-	for i := range goroutines {
+	for i := 0; i < goroutines; i++ {
…
-			for j := range operations {
+			for j := 0; j < operations; j++ {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for i := range goroutines {
go func(id int) {
defer wg.Done()
for j := 0; j < operations; j++ {
for j := range operations {
// Test concurrent item operations
for i := 0; i < goroutines; i++ {
go func(id int) {
defer wg.Done()
for j := 0; j < operations; j++ {
// Test concurrent item operations

value := fmt.Sprintf("value-%d-%d", id, j)
cache.SetItem(uint64(j), &value)
Expand Down
4 changes: 2 additions & 2 deletions pkg/cmd/run_node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ func TestParseFlags(t *testing.T) {

testCases := []struct {
name string
got interface{}
expected interface{}
got any
expected any
}{
{"RootDir", nodeConfig.RootDir, "custom/root/dir"},
{"DBPath", nodeConfig.DBPath, "custom/db/path"},
Expand Down
2 changes: 1 addition & 1 deletion pkg/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ signer:
require.Equal(t, "something/config", cfgFromViper.Signer.SignerPath, "Signer.SignerPath should match YAML")
}

func assertFlagValue(t *testing.T, flags *pflag.FlagSet, name string, expectedValue interface{}) {
func assertFlagValue(t *testing.T, flags *pflag.FlagSet, name string, expectedValue any) {
flag := flags.Lookup(name)
assert.NotNil(t, flag, "Flag %s should exist", name)
if flag != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/config/yaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (c *Config) SaveAsYaml() error {
// helper function to process struct fields recursively
var processFields func(t reflect.Type, prefix string)
processFields = func(t reflect.Type, prefix string) {
for i := 0; i < t.NumField(); i++ {
for i := range t.NumField() {
Comment thread
tac0turtle marked this conversation as resolved.
Outdated
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
field := t.Field(i)

// Skip unexported fields
Expand Down
2 changes: 1 addition & 1 deletion pkg/genesis/io_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func TestLoadAndSaveGenesis(t *testing.T) {
// Verify file contents are valid JSON
fileContent, err := os.ReadFile(filepath.Clean(tmpFile))
assert.NoError(t, err)
var jsonContent map[string]interface{}
var jsonContent map[string]any
err = json.Unmarshal(fileContent, &jsonContent)
assert.NoError(t, err)
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/os/os.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
)

type logger interface {
Info(msg string, keyvals ...interface{})
Info(msg string, keyvals ...any)
}

// TrapSignal catches the SIGTERM/SIGINT and executes cb function. After that it exits
Expand Down
4 changes: 2 additions & 2 deletions pkg/os/os_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import (

type mockLogger struct {
lastMsg string
lastKV []interface{}
lastKV []any
}

func (m *mockLogger) Info(msg string, keyvals ...interface{}) {
func (m *mockLogger) Info(msg string, keyvals ...any) {
m.lastMsg = msg
m.lastKV = keyvals
}
Expand Down
8 changes: 2 additions & 6 deletions pkg/p2p/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,7 @@ func TestBootstrapping(t *testing.T) {
assert := assert.New(t)
logger := log.NewTestLogger(t)

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
clients := startTestNetwork(ctx, t, 4, map[int]hostDescr{
clients := startTestNetwork(t.Context(), t, 4, map[int]hostDescr{
1: {conns: []int{0}},
2: {conns: []int{0, 1}},
3: {conns: []int{0}},
Expand All @@ -105,9 +103,7 @@ func TestDiscovery(t *testing.T) {
assert := assert.New(t)
logger := log.NewTestLogger(t)

ctx, cancel := context.WithCancel(context.Background())
defer cancel()
clients := startTestNetwork(ctx, t, 5, map[int]hostDescr{
clients := startTestNetwork(t.Context(), t, 5, map[int]hostDescr{
1: {conns: []int{0}, chainID: "ORU2"},
2: {conns: []int{0}, chainID: "ORU2"},
3: {conns: []int{1}, chainID: "ORU1"},
Expand Down
7 changes: 4 additions & 3 deletions pkg/p2p/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ func getAddr(sk crypto.PrivKey) (multiaddr.Multiaddr, error) {
if len(id) > 8 {
suffix = id[len(id)-8:]
}
ip := append(net.IP{}, unicastAddr...)
ip := make(net.IP, len(unicastAddr))
copy(ip, unicastAddr)
copy(ip[net.IPv6len-len(suffix):], suffix)
a, err := multiaddr.NewMultiaddr(fmt.Sprintf("/ip6/%s/tcp/4242", ip))
if err != nil {
Expand All @@ -71,7 +72,7 @@ func startTestNetwork(ctx context.Context, t *testing.T, n int, conf map[int]hos
require := require.New(t)

mnet := mocknet.New()
for i := 0; i < n; i++ {
for i := range n {

Copilot AI May 2, 2025

Copy link

Choose a reason for hiding this comment

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

The identifier 'n' is an integer and cannot be used with 'for range' directly. Replace with a loop such as 'for i := 0; i < n; i++' or range over a constructed slice.

Suggested change
for i := range n {
for i := 0; i < n; i++ {

Copilot uses AI. Check for mistakes.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
var descr hostDescr
if d, ok := conf[i]; ok {
descr = d
Expand Down Expand Up @@ -106,7 +107,7 @@ func startTestNetwork(ctx context.Context, t *testing.T, n int, conf map[int]hos
}

clients := make([]*Client, n)
for i := 0; i < n; i++ {
for i := range n {
Comment thread
tac0turtle marked this conversation as resolved.
Outdated
tempDir := filepath.Join(t.TempDir(), fmt.Sprintf("client_%d", i))
ClientInitFiles(t, tempDir)
nodeKey, err := key.GenerateNodeKey()
Expand Down
4 changes: 2 additions & 2 deletions pkg/signer/file/file_signer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,9 +391,9 @@ func TestConcurrentAccess(t *testing.T) {
errChan := make(chan error, numGoRoutines*messageCount)
doneChan := make(chan struct{})

for i := 0; i < numGoRoutines; i++ {
for i := range numGoRoutines {
go func(routineNum int) {
for j := 0; j < messageCount; j++ {
for j := range messageCount {

Copilot AI May 2, 2025

Copy link

Choose a reason for hiding this comment

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

The expression 'numGoRoutines' is an integer; using 'for range' on an integer is invalid. Use 'for i := 0; i < numGoRoutines; i++' instead.

Copilot uses AI. Check for mistakes.

Copilot AI May 2, 2025

Copy link

Choose a reason for hiding this comment

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

Using 'for range' on 'messageCount', which is an integer, is not allowed. Replace with a conventional loop such as 'for j := 0; j < messageCount; j++'.

Copilot uses AI. Check for mistakes.
// Create a unique message per goroutine and iteration
message := fmt.Appendf(nil, "Message %d-%d", routineNum, j)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

Expand Down
Loading