Skip to content

Commit 8c78ece

Browse files
iuwqyiriuwqyir
authored
do not pass around pointers to slices and maps (#174)
### TL;DR Refactored BlockData handling to use value types instead of pointers for slices, improving code clarity and reducing unnecessary pointer indirection. ### What changed? This PR changes the signature of several functions across the codebase to use value types for slices instead of pointers to slices. Specifically: - Changed `*[]common.BlockData` to `[]common.BlockData` in function parameters and return types - Changed `*[]common.Block`, `*[]common.Transaction`, `*[]common.Log`, and `*[]common.Trace` to their value counterparts - Updated all related function calls and implementations to match the new signatures - Simplified nil checks by using `len()` directly on slices instead of checking for nil pointers first - Updated mock implementations to reflect these changes ### How to test? - Run the existing test suite to ensure all functionality works as expected - Verify that the committer and reorg handler components function correctly with the new type signatures - Check that storage operations (insert, get, delete) work properly with the refactored types ### Why make this change? Using pointers to slices was unnecessary and added complexity to the codebase. This change: 1. Improves code readability by removing a layer of indirection 2. Makes the code more idiomatic Go by using value types for slices 3. Simplifies nil checks and length checks on collections 4. Reduces the risk of nil pointer dereferences 5. Makes the API more consistent and easier to understand The change is purely refactoring and doesn't alter any business logic or functionality.
2 parents 31936db + 3f090a9 commit 8c78ece

File tree

11 files changed

+152
-152
lines changed

11 files changed

+152
-152
lines changed

‎internal/orchestrator/committer.go

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func (c *Committer) Start(ctx context.Context) {
6464
log.Error().Err(err).Msg("Error getting block data to commit")
6565
continue
6666
}
67-
if blockDataToCommit == nil || len(*blockDataToCommit) == 0 {
67+
if len(blockDataToCommit) == 0 {
6868
log.Debug().Msg("No block data to commit")
6969
continue
7070
}
@@ -104,7 +104,7 @@ func (c *Committer) getBlockNumbersToCommit() ([]*big.Int, error) {
104104
return blockNumbers, nil
105105
}
106106

107-
func (c *Committer) getSequentialBlockDataToCommit() (*[]common.BlockData, error) {
107+
func (c *Committer) getSequentialBlockDataToCommit() ([]common.BlockData, error) {
108108
blocksToCommit, err := c.getBlockNumbersToCommit()
109109
if err != nil {
110110
return nil, fmt.Errorf("error determining blocks to commit: %v", err)
@@ -117,49 +117,49 @@ func (c *Committer) getSequentialBlockDataToCommit() (*[]common.BlockData, error
117117
if err != nil {
118118
return nil, fmt.Errorf("error fetching blocks to commit: %v", err)
119119
}
120-
if blocksData == nil || len(*blocksData) == 0 {
120+
if len(blocksData) == 0 {
121121
log.Warn().Msgf("Committer didn't find the following range in staging: %v - %v", blocksToCommit[0].Int64(), blocksToCommit[len(blocksToCommit)-1].Int64())
122122
c.handleMissingStagingData(blocksToCommit)
123123
return nil, nil
124124
}
125125

126126
// Sort blocks by block number
127-
sort.Slice(*blocksData, func(i, j int) bool {
128-
return (*blocksData)[i].Block.Number.Cmp((*blocksData)[j].Block.Number) < 0
127+
sort.Slice(blocksData, func(i, j int) bool {
128+
return blocksData[i].Block.Number.Cmp(blocksData[j].Block.Number) < 0
129129
})
130130

131-
if (*blocksData)[0].Block.Number.Cmp(blocksToCommit[0]) != 0 {
132-
return nil, c.handleGap(blocksToCommit[0], (*blocksData)[0].Block)
131+
if blocksData[0].Block.Number.Cmp(blocksToCommit[0]) != 0 {
132+
return nil, c.handleGap(blocksToCommit[0], blocksData[0].Block)
133133
}
134134

135135
var sequentialBlockData []common.BlockData
136-
sequentialBlockData = append(sequentialBlockData, (*blocksData)[0])
137-
expectedBlockNumber := new(big.Int).Add((*blocksData)[0].Block.Number, big.NewInt(1))
136+
sequentialBlockData = append(sequentialBlockData, blocksData[0])
137+
expectedBlockNumber := new(big.Int).Add(blocksData[0].Block.Number, big.NewInt(1))
138138

139-
for i := 1; i < len(*blocksData); i++ {
140-
if (*blocksData)[i].Block.Number.Cmp((*blocksData)[i-1].Block.Number) == 0 {
139+
for i := 1; i < len(blocksData); i++ {
140+
if blocksData[i].Block.Number.Cmp(blocksData[i-1].Block.Number) == 0 {
141141
// Duplicate block, skip -- might happen if block has been polled multiple times
142142
continue
143143
}
144-
if (*blocksData)[i].Block.Number.Cmp(expectedBlockNumber) != 0 {
144+
if blocksData[i].Block.Number.Cmp(expectedBlockNumber) != 0 {
145145
// Note: Gap detected, stop here
146-
log.Warn().Msgf("Gap detected at block %s, committing until %s", expectedBlockNumber.String(), (*blocksData)[i-1].Block.Number.String())
146+
log.Warn().Msgf("Gap detected at block %s, committing until %s", expectedBlockNumber.String(), blocksData[i-1].Block.Number.String())
147147
// increment the gap counter in prometheus
148148
metrics.GapCounter.Inc()
149149
// record the first missed block number in prometheus
150-
metrics.MissedBlockNumbers.Set(float64((*blocksData)[0].Block.Number.Int64()))
150+
metrics.MissedBlockNumbers.Set(float64(blocksData[0].Block.Number.Int64()))
151151
break
152152
}
153-
sequentialBlockData = append(sequentialBlockData, (*blocksData)[i])
153+
sequentialBlockData = append(sequentialBlockData, blocksData[i])
154154
expectedBlockNumber.Add(expectedBlockNumber, big.NewInt(1))
155155
}
156156

157-
return &sequentialBlockData, nil
157+
return sequentialBlockData, nil
158158
}
159159

160-
func (c *Committer) commit(blockData *[]common.BlockData) error {
161-
blockNumbers := make([]*big.Int, len(*blockData))
162-
for i, block := range *blockData {
160+
func (c *Committer) commit(blockData []common.BlockData) error {
161+
blockNumbers := make([]*big.Int, len(blockData))
162+
for i, block := range blockData {
163163
blockNumbers[i] = block.Block.Number
164164
}
165165
log.Debug().Msgf("Committing %d blocks", len(blockNumbers))
@@ -175,16 +175,16 @@ func (c *Committer) commit(blockData *[]common.BlockData) error {
175175
}
176176

177177
// Find highest block number from committed blocks
178-
highestBlockNumber := (*blockData)[0].Block.Number
179-
for _, block := range *blockData {
178+
highestBlockNumber := blockData[0].Block.Number
179+
for _, block := range blockData {
180180
if block.Block.Number.Cmp(highestBlockNumber) > 0 {
181181
highestBlockNumber = block.Block.Number
182182
}
183183
}
184184
c.lastCommittedBlock = new(big.Int).Set(highestBlockNumber)
185185

186186
// Update metrics for successful commits
187-
metrics.SuccessfulCommits.Add(float64(len(*blockData)))
187+
metrics.SuccessfulCommits.Add(float64(len(blockData)))
188188
metrics.LastCommittedBlock.Set(float64(highestBlockNumber.Int64()))
189189
return nil
190190
}

‎internal/orchestrator/committer_test.go

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -252,13 +252,13 @@ func TestGetSequentialBlockDataToCommit(t *testing.T) {
252252
mockStagingStorage.EXPECT().GetStagingData(storage.QueryFilter{
253253
ChainId: chainID,
254254
BlockNumbers: []*big.Int{big.NewInt(101), big.NewInt(102), big.NewInt(103)},
255-
}).Return(&blockData, nil)
255+
}).Return(blockData, nil)
256256

257257
result, err := committer.getSequentialBlockDataToCommit()
258258

259259
assert.NoError(t, err)
260260
assert.NotNil(t, result)
261-
assert.Equal(t, 3, len(*result))
261+
assert.Equal(t, 3, len(result))
262262
}
263263

264264
func TestGetSequentialBlockDataToCommitWithDuplicateBlocks(t *testing.T) {
@@ -288,16 +288,16 @@ func TestGetSequentialBlockDataToCommitWithDuplicateBlocks(t *testing.T) {
288288
mockStagingStorage.EXPECT().GetStagingData(storage.QueryFilter{
289289
ChainId: chainID,
290290
BlockNumbers: []*big.Int{big.NewInt(101), big.NewInt(102), big.NewInt(103)},
291-
}).Return(&blockData, nil)
291+
}).Return(blockData, nil)
292292

293293
result, err := committer.getSequentialBlockDataToCommit()
294294

295295
assert.NoError(t, err)
296296
assert.NotNil(t, result)
297-
assert.Equal(t, 3, len(*result))
298-
assert.Equal(t, big.NewInt(101), (*result)[0].Block.Number)
299-
assert.Equal(t, big.NewInt(102), (*result)[1].Block.Number)
300-
assert.Equal(t, big.NewInt(103), (*result)[2].Block.Number)
297+
assert.Equal(t, 3, len(result))
298+
assert.Equal(t, big.NewInt(101), result[0].Block.Number)
299+
assert.Equal(t, big.NewInt(102), result[1].Block.Number)
300+
assert.Equal(t, big.NewInt(103), result[2].Block.Number)
301301
}
302302

303303
func TestCommit(t *testing.T) {
@@ -317,10 +317,10 @@ func TestCommit(t *testing.T) {
317317
{Block: common.Block{Number: big.NewInt(102)}},
318318
}
319319

320-
mockMainStorage.EXPECT().InsertBlockData(&blockData).Return(nil)
321-
mockStagingStorage.EXPECT().DeleteStagingData(&blockData).Return(nil)
320+
mockMainStorage.EXPECT().InsertBlockData(blockData).Return(nil)
321+
mockStagingStorage.EXPECT().DeleteStagingData(blockData).Return(nil)
322322

323-
err := committer.commit(&blockData)
323+
err := committer.commit(blockData)
324324

325325
assert.NoError(t, err)
326326
}
@@ -381,9 +381,9 @@ func TestStartCommitter(t *testing.T) {
381381
{Block: common.Block{Number: big.NewInt(101)}},
382382
{Block: common.Block{Number: big.NewInt(102)}},
383383
}
384-
mockStagingStorage.On("GetStagingData", mock.Anything).Return(&blockData, nil)
385-
mockMainStorage.On("InsertBlockData", &blockData).Return(nil)
386-
mockStagingStorage.On("DeleteStagingData", &blockData).Return(nil)
384+
mockStagingStorage.On("GetStagingData", mock.Anything).Return(blockData, nil)
385+
mockMainStorage.On("InsertBlockData", blockData).Return(nil)
386+
mockStagingStorage.On("DeleteStagingData", blockData).Return(nil)
387387

388388
// Start the committer in a goroutine
389389
go committer.Start(context.Background())
@@ -414,9 +414,9 @@ func TestCommitterRespectsSIGTERM(t *testing.T) {
414414
{Block: common.Block{Number: big.NewInt(101)}},
415415
{Block: common.Block{Number: big.NewInt(102)}},
416416
}
417-
mockStagingStorage.On("GetStagingData", mock.Anything).Return(&blockData, nil)
418-
mockMainStorage.On("InsertBlockData", &blockData).Return(nil)
419-
mockStagingStorage.On("DeleteStagingData", &blockData).Return(nil)
417+
mockStagingStorage.On("GetStagingData", mock.Anything).Return(blockData, nil)
418+
mockMainStorage.On("InsertBlockData", blockData).Return(nil)
419+
mockStagingStorage.On("DeleteStagingData", blockData).Return(nil)
420420

421421
// Create a context that we can cancel
422422
ctx, cancel := context.WithCancel(context.Background())
@@ -480,7 +480,7 @@ func TestHandleMissingStagingData(t *testing.T) {
480480
mockStagingStorage.EXPECT().GetStagingData(storage.QueryFilter{
481481
ChainId: chainID,
482482
BlockNumbers: []*big.Int{big.NewInt(0), big.NewInt(1), big.NewInt(2), big.NewInt(3), big.NewInt(4)},
483-
}).Return(&blockData, nil)
483+
}).Return(blockData, nil)
484484

485485
result, err := committer.getSequentialBlockDataToCommit()
486486

@@ -524,7 +524,7 @@ func TestHandleMissingStagingDataIsPolledWithCorrectBatchSize(t *testing.T) {
524524
mockStagingStorage.EXPECT().GetStagingData(storage.QueryFilter{
525525
ChainId: chainID,
526526
BlockNumbers: []*big.Int{big.NewInt(0), big.NewInt(1), big.NewInt(2), big.NewInt(3), big.NewInt(4)},
527-
}).Return(&blockData, nil)
527+
}).Return(blockData, nil)
528528

529529
result, err := committer.getSequentialBlockDataToCommit()
530530

‎internal/orchestrator/reorg_handler.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ func (rh *ReorgHandler) findReorgedBlockNumbers(blockHeadersDescending []common.
194194
continueCheckingForReorgs := false
195195
for i := 0; i < len(blockHeadersDescending); i++ {
196196
blockHeader := blockHeadersDescending[i]
197-
fetchedBlock, ok := (*newBlocksByNumber)[blockHeader.Number.String()]
197+
fetchedBlock, ok := newBlocksByNumber[blockHeader.Number.String()]
198198
if !ok {
199199
return fmt.Errorf("block not found: %s", blockHeader.Number.String())
200200
}
@@ -220,7 +220,7 @@ func (rh *ReorgHandler) findReorgedBlockNumbers(blockHeadersDescending []common.
220220
return nil
221221
}
222222

223-
func (rh *ReorgHandler) getNewBlocksByNumber(blockHeaders []common.BlockHeader) (*map[string]common.Block, error) {
223+
func (rh *ReorgHandler) getNewBlocksByNumber(blockHeaders []common.BlockHeader) (map[string]common.Block, error) {
224224
blockNumbers := make([]*big.Int, 0, len(blockHeaders))
225225
for _, header := range blockHeaders {
226226
blockNumbers = append(blockNumbers, header.Number)
@@ -257,7 +257,7 @@ func (rh *ReorgHandler) getNewBlocksByNumber(blockHeaders []common.BlockHeader)
257257
fetchedBlocksByNumber[blockResult.BlockNumber.String()] = blockResult.Data
258258
}
259259
}
260-
return &fetchedBlocksByNumber, nil
260+
return fetchedBlocksByNumber, nil
261261
}
262262

263263
func (rh *ReorgHandler) handleReorg(reorgedBlockNumbers []*big.Int) error {
@@ -281,7 +281,7 @@ func (rh *ReorgHandler) handleReorg(reorgedBlockNumbers []*big.Int) error {
281281
if err := rh.storage.MainStorage.DeleteBlockData(rh.rpc.GetChainID(), blocksToDelete); err != nil {
282282
return fmt.Errorf("error deleting data for blocks %v: %w", blocksToDelete, err)
283283
}
284-
if err := rh.storage.MainStorage.InsertBlockData(&data); err != nil {
284+
if err := rh.storage.MainStorage.InsertBlockData(data); err != nil {
285285
return fmt.Errorf("error saving data to main storage: %w", err)
286286
}
287287
return nil

‎internal/orchestrator/reorg_handler_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -614,8 +614,8 @@ func TestHandleReorgWithSingleBlockReorg(t *testing.T) {
614614
mockMainStorage.EXPECT().DeleteBlockData(big.NewInt(1), mock.MatchedBy(func(blocks []*big.Int) bool {
615615
return len(blocks) == 1
616616
})).Return(nil)
617-
mockMainStorage.EXPECT().InsertBlockData(mock.MatchedBy(func(data *[]common.BlockData) bool {
618-
return data != nil && len(*data) == 1
617+
mockMainStorage.EXPECT().InsertBlockData(mock.MatchedBy(func(data []common.BlockData) bool {
618+
return len(data) == 1
619619
})).Return(nil)
620620

621621
handler := NewReorgHandler(mockRPC, mockStorage)
@@ -682,8 +682,8 @@ func TestHandleReorgWithLatestBlockReorged(t *testing.T) {
682682
mockMainStorage.EXPECT().DeleteBlockData(big.NewInt(1), mock.MatchedBy(func(blocks []*big.Int) bool {
683683
return len(blocks) == 8
684684
})).Return(nil)
685-
mockMainStorage.EXPECT().InsertBlockData(mock.MatchedBy(func(data *[]common.BlockData) bool {
686-
return data != nil && len(*data) == 8
685+
mockMainStorage.EXPECT().InsertBlockData(mock.MatchedBy(func(data []common.BlockData) bool {
686+
return len(data) == 8
687687
})).Return(nil)
688688

689689
handler := NewReorgHandler(mockRPC, mockStorage)
@@ -746,8 +746,8 @@ func TestHandleReorgWithManyBlocks(t *testing.T) {
746746
mockMainStorage.EXPECT().DeleteBlockData(big.NewInt(1), mock.MatchedBy(func(blocks []*big.Int) bool {
747747
return len(blocks) == 5
748748
})).Return(nil)
749-
mockMainStorage.EXPECT().InsertBlockData(mock.MatchedBy(func(data *[]common.BlockData) bool {
750-
return data != nil && len(*data) == 5
749+
mockMainStorage.EXPECT().InsertBlockData(mock.MatchedBy(func(data []common.BlockData) bool {
750+
return len(data) == 5
751751
})).Return(nil)
752752

753753
handler := NewReorgHandler(mockRPC, mockStorage)

‎internal/rpc/rpc.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -212,29 +212,29 @@ func (rpc *Client) setChainID() error {
212212

213213
func (rpc *Client) GetFullBlocks(blockNumbers []*big.Int) []GetFullBlockResult {
214214
var wg sync.WaitGroup
215-
var blocks *[]RPCFetchBatchResult[common.RawBlock]
216-
var logs *[]RPCFetchBatchResult[common.RawLogs]
217-
var traces *[]RPCFetchBatchResult[common.RawTraces]
218-
var receipts *[]RPCFetchBatchResult[common.RawReceipts]
215+
var blocks []RPCFetchBatchResult[common.RawBlock]
216+
var logs []RPCFetchBatchResult[common.RawLogs]
217+
var traces []RPCFetchBatchResult[common.RawTraces]
218+
var receipts []RPCFetchBatchResult[common.RawReceipts]
219219
wg.Add(2)
220220

221221
go func() {
222222
defer wg.Done()
223223
result := RPCFetchBatch[common.RawBlock](rpc, blockNumbers, "eth_getBlockByNumber", GetBlockWithTransactionsParams)
224-
blocks = &result
224+
blocks = result
225225
}()
226226

227227
if rpc.supportsBlockReceipts {
228228
go func() {
229229
defer wg.Done()
230230
result := RPCFetchInBatches[common.RawReceipts](rpc, blockNumbers, rpc.blocksPerRequest.Receipts, config.Cfg.RPC.BlockReceipts.BatchDelay, "eth_getBlockReceipts", GetBlockReceiptsParams)
231-
receipts = &result
231+
receipts = result
232232
}()
233233
} else {
234234
go func() {
235235
defer wg.Done()
236236
result := RPCFetchInBatches[common.RawLogs](rpc, blockNumbers, rpc.blocksPerRequest.Logs, config.Cfg.RPC.Logs.BatchDelay, "eth_getLogs", GetLogsParams)
237-
logs = &result
237+
logs = result
238238
}()
239239
}
240240

@@ -243,7 +243,7 @@ func (rpc *Client) GetFullBlocks(blockNumbers []*big.Int) []GetFullBlockResult {
243243
go func() {
244244
defer wg.Done()
245245
result := RPCFetchInBatches[common.RawTraces](rpc, blockNumbers, rpc.blocksPerRequest.Traces, config.Cfg.RPC.Traces.BatchDelay, "trace_block", TraceBlockParams)
246-
traces = &result
246+
traces = result
247247
}()
248248
}
249249

‎internal/rpc/serializer.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,17 +11,17 @@ import (
1111
"github.com/thirdweb-dev/indexer/internal/common"
1212
)
1313

14-
func SerializeFullBlocks(chainId *big.Int, blocks *[]RPCFetchBatchResult[common.RawBlock], logs *[]RPCFetchBatchResult[common.RawLogs], traces *[]RPCFetchBatchResult[common.RawTraces], receipts *[]RPCFetchBatchResult[common.RawReceipts]) []GetFullBlockResult {
14+
func SerializeFullBlocks(chainId *big.Int, blocks []RPCFetchBatchResult[common.RawBlock], logs []RPCFetchBatchResult[common.RawLogs], traces []RPCFetchBatchResult[common.RawTraces], receipts []RPCFetchBatchResult[common.RawReceipts]) []GetFullBlockResult {
1515
if blocks == nil {
1616
return []GetFullBlockResult{}
1717
}
18-
results := make([]GetFullBlockResult, 0, len(*blocks))
18+
results := make([]GetFullBlockResult, 0, len(blocks))
1919

2020
rawLogsMap := mapBatchResultsByBlockNumber[common.RawLogs](logs)
2121
rawReceiptsMap := mapBatchResultsByBlockNumber[common.RawReceipts](receipts)
2222
rawTracesMap := mapBatchResultsByBlockNumber[common.RawTraces](traces)
2323

24-
for _, rawBlock := range *blocks {
24+
for _, rawBlock := range blocks {
2525
result := GetFullBlockResult{
2626
BlockNumber: rawBlock.BlockNumber,
2727
}
@@ -45,7 +45,7 @@ func SerializeFullBlocks(chainId *big.Int, blocks *[]RPCFetchBatchResult[common.
4545
if rawReceipts.Error != nil {
4646
result.Error = rawReceipts.Error
4747
} else {
48-
result.Data.Logs = serializeLogsFromReceipts(chainId, &rawReceipts.Result, result.Data.Block)
48+
result.Data.Logs = serializeLogsFromReceipts(chainId, rawReceipts.Result, result.Data.Block)
4949
result.Data.Transactions = serializeTransactions(chainId, rawBlock.Result["transactions"].([]interface{}), blockTimestamp, &rawReceipts.Result)
5050
}
5151
} else {
@@ -75,12 +75,12 @@ func SerializeFullBlocks(chainId *big.Int, blocks *[]RPCFetchBatchResult[common.
7575
return results
7676
}
7777

78-
func mapBatchResultsByBlockNumber[T any](results *[]RPCFetchBatchResult[T]) map[string]*RPCFetchBatchResult[T] {
78+
func mapBatchResultsByBlockNumber[T any](results []RPCFetchBatchResult[T]) map[string]*RPCFetchBatchResult[T] {
7979
if results == nil {
8080
return make(map[string]*RPCFetchBatchResult[T], 0)
8181
}
82-
resultsMap := make(map[string]*RPCFetchBatchResult[T], len(*results))
83-
for _, result := range *results {
82+
resultsMap := make(map[string]*RPCFetchBatchResult[T], len(results))
83+
for _, result := range results {
8484
resultsMap[result.BlockNumber.String()] = &result
8585
}
8686
return resultsMap
@@ -278,13 +278,13 @@ func ExtractFunctionSelector(s string) string {
278278
return s[0:10]
279279
}
280280

281-
func serializeLogsFromReceipts(chainId *big.Int, rawReceipts *[]map[string]interface{}, block common.Block) []common.Log {
281+
func serializeLogsFromReceipts(chainId *big.Int, rawReceipts []map[string]interface{}, block common.Block) []common.Log {
282282
logs := make([]common.Log, 0)
283283
if rawReceipts == nil {
284284
return logs
285285
}
286286

287-
for _, receipt := range *rawReceipts {
287+
for _, receipt := range rawReceipts {
288288
rawLogs, ok := receipt["logs"].([]interface{})
289289
if !ok {
290290
log.Debug().Msgf("Failed to serialize logs: %v", receipt["logs"])

0 commit comments

Comments
 (0)