refactor: break up transformer and rpc package logic into smaller func and add unit tests #1313

Monika-Bitfly · 2025-02-05T17:04:42Z

Changes:

Refactored transformer.go functions in the /executionlayer package by breaking them into smaller, more readable functions
Simplified erigon.go and geth.go functions in the /rpc package by splitting them into smaller functions and introduced utils.go to share common functionality between them
Added unit tests utils_test.go and erigon_test.go in the /rpc package
Added unit tests transformer_test.go in the /executionlayer package

… redability

…oved redability

…nto m/refactor-indexing-v2

…alue

fix

…Values & TestCalculateTxFee methods

…nto m/refactor-indexing-v2

… calculateUncleReward & verifyName methods

AdaptorV1 data.Store metadataupdates.Store TransformFunc

…ly & getERC20ContractDecimals method

…nto m/refactor-indexing-v2

…ctor-indexing-v2

Tangui-Bitfly

There is a lot of good things in that pr but I'm wondering if some changes go maybe too far (too many little functions, too many tests that test not exported func without direct link to usage, ...) and will be a burden in the long run.

Tangui-Bitfly · 2025-02-07T14:43:44Z

backend/internal/th/ethereum.go

@@ -207,6 +207,16 @@ func (b *BlockchainBackend) DeployContract(t *testing.T, contractData []byte) co
 	return receipt.ContractAddress
 }

+func (b *BlockchainBackend) NewERC20Metadata(t *testing.T, address common.Address) (*contracts.IERC20Metadata, error) {


I don't really see the point of this function, it's just a wrapper around contracts.NewIERC20Metadata but without adding anything.

Tangui-Bitfly · 2025-02-07T14:48:51Z

backend/pkg/commons/rpc/erigon.go

+	uncles := getBlockUncles(block.Uncles())
+	baseFee := getBaseFee(block)
+	blobGasUsed := getBlobGasUsed(block)
+	excessBlobGas := getExcessBlobGas(block)


those can be moved when we create types.Eth1Block like

&types.Eth1Block{ ... BaseFee: getBaseFee(block), ... }

Tangui-Bitfly · 2025-02-07T14:49:22Z

backend/pkg/commons/rpc/erigon.go

+		to := getReceiver(tx)
+		maxFeePerBlobGas := getMaxFeePerBlobGas(tx)
+		blobVersionedHashes := getBlobVersionedHashes(tx)
+		blobGasPrice := getBlobGasPrice(receipt)


same, inline when possible

Tangui-Bitfly · 2025-02-07T14:53:43Z

backend/pkg/commons/rpc/erigon_test.go

+
+// TestGetInternalTxs tests the getInternalTxs function
+// which returns the internal transactions of a transaction at a given position
+func TestGetInternalTxs(t *testing.T) {


do we really want those comments on top of each tests ? this doesn't bring anything new IMO

Tangui-Bitfly · 2025-02-07T14:57:37Z

backend/pkg/commons/rpc/erigon_test.go

+
+// TestGetMaxFeePerBlobGas tests the getMaxFeePerBlobGas function
+// which extracts the max fee per blob gas from a transaction
+func TestGetMaxFeePerBlobGas(t *testing.T) {


Do we really want to test each individual function like that ? Too much tests is like too much codes, harder to maintain, more complex and prone to brake.
I mean what this test is really testing ? Following this logic we could do a test for each field of a block. IMO it's better to create one test where we test each field all together.

Tangui-Bitfly · 2025-02-07T15:53:23Z

backend/pkg/commons/rpc/utils_test.go

+			if err != nil && tt.expectedErr == nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if err == nil && tt.expectedErr != nil {


Tangui-Bitfly · 2025-02-07T15:53:32Z

backend/pkg/commons/rpc/utils_test.go

+			if err != nil && tt.expectedErr == nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if err == nil && tt.expectedErr != nil {


Tangui-Bitfly · 2025-02-07T15:54:54Z

backend/pkg/executionlayer/transformer.go

+	return v
+}
+
+func updateITxStatus(internalTx []*types.Eth1InternalTransaction, indexedTx *types.Eth1TransactionIndexed) {


minor suggestion but the signature would better if reversed and internalTx should be plural

func updateITxStatus(indexedTx *types.Eth1TransactionIndexed, internals []*types.Eth1InternalTransaction)

Tangui-Bitfly · 2025-02-07T15:57:36Z

backend/pkg/executionlayer/transformer.go

@@ -320,7 +269,8 @@ func TransformITx(chainID string, block *types.Eth1Block, res *IndexedBlock) err
 	var transactions []data.InternalWithIndexes
 	for i, tx := range block.GetTransactions() {
 		for j, itx := range tx.GetItx() {
-			if itx.Path == "0" || itx.Path == "[]" || bytes.Equal(itx.Value, []byte{0x0}) { // skip top level and empty calls
+			// skip top level and empty calls


this comment should be inside isValidItx func IMO

Tangui-Bitfly · 2025-02-07T16:00:31Z

backend/pkg/executionlayer/transformer_test.go

+			if (result == nil && tt.expected != nil) || (result != nil && tt.expected == nil) {
+				t.Errorf("got %v, want %v", result, tt.expected)
+			} else if result != nil && tt.expected != nil && result.Error() != tt.expected.Error() {
+				t.Errorf("got %v, want %v", result, tt.expected)


Tangui-Bitfly and others added 30 commits January 30, 2025 15:45

refactor(execution layer): extract el indexing it own packages

b06f30c

chore: contract simplification

df7c389

feat(database transaction): add transaction to database package

504de9a

fix: rename database.Transaction to database.Batch

6b06d7b

fix: check max uncles len

0bf7bbf

refactor: extract logic into separate functions for improved readability

3f74ed5

refactor: break down transformer logic into smaller func for improved…

cae25cb

… redability

refactor(indexer): remove even more dependancies with database

d14bd52

refactor: break down erigon and geth logic into smaller func for impr…

2efbee2

…oved redability

add test for erc20 transform

ae0831f

refactor: add utils.go for code reuse

1634979

Merge branch 'refactor-indexing' of github.com:gobitfly/beaconchain i…

d767adb

…nto m/refactor-indexing-v2

test: add tests for getTxRecipient, getMethodSignature & updateITxStatus

0d4bb82

test: add tests for isValidERC20Log, getLogTopics & getERC20TransferV…

9f1af82

…alue

fix: fix isInvalidItx func

44a6489

fix: update isInvalidItx func

848da84

test: add tests for isValidERC721Log & isValidERC1155Log methods

6acf6a2

fix: update isValidERC721Log func

f20a36a

refactor(execution layer): extract el indexing it own packages

fc7de46

chore: contract simplification

2172697

feat(database transaction): add transaction to database package

29c46c7

fix: rename database.Transaction to database.Batch

b97f7d2

fix: check max uncles len

9bec588

refactor(indexer): remove even more dependancies with database

3380e72

add test for erc20 transform

6c491cd

final transformer refactor

c991ce3

add more tests

79e8597

fix

test: add tests for isValidItx, getTokenID & getContractAddress methods

5d15272

test: add tests for TestGetERC1155TransferIDs, TestGetERC1155Transfer…

da3daa1

…Values & TestCalculateTxFee methods

more tests

a63ebab

Monika-Bitfly and others added 20 commits February 4, 2025 16:13

Merge branch 'refactor-indexing' of github.com:gobitfly/beaconchain i…

8f8d4e8

…nto m/refactor-indexing-v2

refactor: update isBlobTx func

b2e9786

test: add tests for calculateMevFromBlock, calculateBlockUncleReward,…

4f52647

… calculateUncleReward & verifyName methods

test: add rpc/utils tests

6afef03

fix missing chainID

6e2773f

simplify pr

a5de2d4

refactor: update parseAddressBalance function

e1d61ab

test: add tests for TestGetTokens & TestParseAddressBalance methods

ced4961

comments

5a9c002

AdaptorV1 data.Store metadataupdates.Store TransformFunc

test: add function-level comments to tests

96d9352

test: add tests for getBlockUncles method

dbd9038

remove unused func

1a39ae4

refactor: updated ERC20 contract handlers name

1bfad7f

fix merge map

b8ad60b

test: add tests for getERC20ContractSymbol, getERC20ContractTotalSupp…

6d4a117

…ly & getERC20ContractDecimals method

Merge branch 'refactor-indexing' of github.com:gobitfly/beaconchain i…

353a2f8

…nto m/refactor-indexing-v2

refactor: update getTxRecipient func

6e067ee

refactor: update ERC20 contract test func name

eb469a5

fix: lint

5c8d3c6

test: add rpc/erigon tests

afb5116

Monika-Bitfly marked this pull request as ready for review February 5, 2025 18:55

Monika-Bitfly requested review from Tangui-Bitfly and mccreedy-bitfly February 5, 2025 18:55

Merge branch 'staging' of github.com:gobitfly/beaconchain into m/refa…

802c420

…ctor-indexing-v2

Tangui-Bitfly reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: break up transformer and rpc package logic into smaller func and add unit tests #1313

refactor: break up transformer and rpc package logic into smaller func and add unit tests #1313

Monika-Bitfly commented Feb 5, 2025

Tangui-Bitfly left a comment

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

Tangui-Bitfly Feb 7, 2025

refactor: break up transformer and rpc package logic into smaller func and add unit tests #1313

Are you sure you want to change the base?

refactor: break up transformer and rpc package logic into smaller func and add unit tests #1313

Conversation

Monika-Bitfly commented Feb 5, 2025

Tangui-Bitfly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment