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.

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

Jump to bottom

fix: allow processing v2 inbound trackers for gateway calls initiated by smart contracts #3226

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

fix: allow processing v2 inbound trackers for gateway calls initiated by smart contracts #3226

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7b856c7
fix destination check
lumtis Nov 27, 2024
be1179b
add inbound tracker test with contract
lumtis Nov 27, 2024
93efb98
fix tracker v2 processing
lumtis Nov 27, 2024
8d5caac
e2e test
lumtis Nov 27, 2024
c35c086
prevent hiding smart contract error
lumtis Nov 27, 2024
4cd508d
fix tests
lumtis Nov 27, 2024
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
2 changes: 2 additions & 0 deletions e2e/e2etests/test_deploy_contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func deployZEVMTestDApp(r *runner.E2ERunner) (ethcommon.Address, error) {
r.ZEVMAuth,
r.ZEVMClient,
true,
r.GatewayEVMAddr,
)
if err != nil {
return addr, err
Expand All @@ -66,6 +67,7 @@ func deployEVMTestDApp(r *runner.E2ERunner) (ethcommon.Address, error) {
r.EVMAuth,
r.EVMClient,
false,
r.GatewayEVMAddr,
)
if err != nil {
return addr, err
Expand Down
28 changes: 28 additions & 0 deletions e2e/e2etests/test_inbound_trackers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,32 @@ func TestInboundTrackers(r *runner.E2ERunner, args []string) {
)
addTrackerAndWaitForCCTX(coin.CoinType_NoAssetCall, tx.Hash().Hex())
r.Logger.Print("🍾v2 call observed")

// set value of the payable transactions
previousValue := r.EVMAuth.Value
r.EVMAuth.Value = amount

// send v2 deposit through contract
r.Logger.Print("🏃test v2 deposit through contract")
tx, err := r.TestDAppV2EVM.GatewayDeposit(r.EVMAuth, r.EVMAddress())
require.NoError(r, err)
addTrackerAndWaitForCCTX(coin.CoinType_Gas, tx.Hash().Hex())
r.Logger.Print("🍾v2 deposit through contract observed")

// send v2 deposit and call through contract
r.Logger.Print("🏃test v2 deposit and call through contract")
tx, err = r.TestDAppV2EVM.GatewayDepositAndCall(r.EVMAuth, r.TestDAppV2ZEVMAddr, []byte(randomPayload(r)))
require.NoError(r, err)
addTrackerAndWaitForCCTX(coin.CoinType_Gas, tx.Hash().Hex())
r.Logger.Print("🍾v2 deposit and call through contract observed")

// reset the value of the payable transactions
r.EVMAuth.Value = previousValue

// send v2 call through contract
r.Logger.Print("🏃test v2 call through contract")
tx, err = r.TestDAppV2EVM.GatewayCall(r.EVMAuth, r.TestDAppV2ZEVMAddr, []byte(randomPayload(r)))
require.NoError(r, err)
addTrackerAndWaitForCCTX(coin.CoinType_NoAssetCall, tx.Hash().Hex())
r.Logger.Print("🍾v2 call through contract observed")
}
2 changes: 1 addition & 1 deletion e2e/runner/balances.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func (r *E2ERunner) GetAccountBalances(skipBTC bool) (AccountBalances, error) {
// solana
var solSOL *big.Int
var solSPL *big.Int
if r.Account.SolanaAddress != "" && r.Account.SolanaPrivateKey != "" {
if r.Account.SolanaAddress != "" && r.Account.SolanaPrivateKey != "" && r.SolanaClient != nil {
solanaAddr := solana.MustPublicKeyFromBase58(r.Account.SolanaAddress.String())
privateKey := solana.MustPrivateKeyFromBase58(r.Account.SolanaPrivateKey.String())
solSOLBalance, err := r.SolanaClient.GetBalance(
Expand Down
2 changes: 1 addition & 1 deletion e2e/runner/v2_setup_evm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func (r *E2ERunner) SetupEVMV2() {
require.NoError(r, err)

// deploy test dapp v2
testDAppV2Addr, txTestDAppV2, _, err := testdappv2.DeployTestDAppV2(r.EVMAuth, r.EVMClient, false)
testDAppV2Addr, txTestDAppV2, _, err := testdappv2.DeployTestDAppV2(r.EVMAuth, r.EVMClient, false, r.GatewayEVMAddr)
require.NoError(r, err)

r.TestDAppV2EVMAddr = testDAppV2Addr
Expand Down
7 changes: 6 additions & 1 deletion e2e/runner/v2_setup_zeta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,12 @@ func (r *E2ERunner) SetZEVMContractsV2() {
require.NoError(r, err)

// deploy test dapp v2
testDAppV2Addr, txTestDAppV2, _, err := testdappv2.DeployTestDAppV2(r.ZEVMAuth, r.ZEVMClient, true)
testDAppV2Addr, txTestDAppV2, _, err := testdappv2.DeployTestDAppV2(
r.ZEVMAuth,
r.ZEVMClient,
true,
r.GatewayEVMAddr,
)
require.NoError(r, err)

r.TestDAppV2ZEVMAddr = testDAppV2Addr
Expand Down
67 changes: 67 additions & 0 deletions pkg/contracts/testdappv2/TestDAppV2.abi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
"internalType": "bool",
"name": "isZetaChain_",
"type": "bool"
},
{
"internalType": "address",
"name": "gateway_",
"type": "address"
}
],
"stateMutability": "nonpayable",
Expand Down Expand Up @@ -97,6 +102,68 @@
"stateMutability": "payable",
"type": "function"
},
{
"inputs": [],
"name": "gateway",
"outputs": [
{
"internalType": "address",
"name": "",
"type": "address"
}
],
"stateMutability": "view",
"type": "function"
},
{
"inputs": [
{
"internalType": "address",
"name": "dst",
"type": "address"
},
{
"internalType": "bytes",
"name": "payload",
"type": "bytes"
}
],
"name": "gatewayCall",
"outputs": [],
"stateMutability": "nonpayable",
"type": "function"
},
{
"inputs": [
{
"internalType": "address",
"name": "dst",
"type": "address"
}
],
"name": "gatewayDeposit",
"outputs": [],
"stateMutability": "payable",
"type": "function"
},
{
"inputs": [
{
"internalType": "address",
"name": "dst",
"type": "address"
},
{
"internalType": "bytes",
"name": "payload",
"type": "bytes"
}
],
"name": "gatewayDepositAndCall",
"outputs": [],
"stateMutability": "payable",
"type": "function"
},
{
"inputs": [
{
Expand Down
2 changes: 1 addition & 1 deletion pkg/contracts/testdappv2/TestDAppV2.bin
View file
Open in desktop

Large diffs are not rendered by default.

102 changes: 98 additions & 4 deletions pkg/contracts/testdappv2/TestDAppV2.go
View file
Open in desktop

Large diffs are not rendered by default.

69 changes: 68 additions & 1 deletion pkg/contracts/testdappv2/TestDAppV2.json
View file
Open in desktop

Large diffs are not rendered by default.

44 changes: 43 additions & 1 deletion pkg/contracts/testdappv2/TestDAppV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,26 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.26;

struct RevertOptions {
address revertAddress;
bool callOnRevert;
address abortAddress;
bytes revertMessage;
uint256 onRevertGasLimit;
}

interface IGatewayEVM {
function deposit(address receiver, RevertOptions calldata revertOptions) external payable;
function depositAndCall(
address receiver,
bytes calldata payload,
RevertOptions calldata revertOptions
)
external
payable;
function call(address receiver, bytes calldata payload, RevertOptions calldata revertOptions) external;
}

interface IERC20 {
function transferFrom(address sender, address recipient, uint256 amount) external returns (bool);
}
Expand All @@ -14,6 +34,9 @@ contract TestDAppV2 {
// define if the chain is ZetaChain
bool immutable public isZetaChain;

// address of the gateway
address immutable public gateway;

struct zContext {
bytes origin;
address sender;
Expand Down Expand Up @@ -44,8 +67,9 @@ contract TestDAppV2 {
mapping(bytes32 => uint256) public amountWithMessage;

// the constructor is used to determine if the chain is ZetaChain
constructor(bool isZetaChain_) {
constructor(bool isZetaChain_, address gateway_) {
isZetaChain = isZetaChain_;
gateway = gateway_;
}

// return the index used for the "WithMessage" mapping when the message for calls is empty
Expand Down Expand Up @@ -144,6 +168,24 @@ contract TestDAppV2 {
return "";
}

// deposit through Gateway EVM
function gatewayDeposit(address dst) external payable {
require(!isZetaChain);
IGatewayEVM(gateway).deposit{value: msg.value}(dst, RevertOptions(msg.sender, false, address(0), "", 0));
}
Comment on lines +171 to +175
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate the dst address in gatewayDeposit function

To prevent potential issues, add a check to ensure that the dst (destination) address is not the zero address. This adds a layer of safety against inadvertent misuse.

Apply this diff to include the validation:

 function gatewayDeposit(address dst) external payable {
     require(!isZetaChain);
+    require(dst != address(0), "Invalid destination address");
     IGatewayEVM(gateway).deposit{value: msg.value}(dst, RevertOptions(msg.sender, false, address(0), "", 0));
 }
📝 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
// deposit through Gateway EVM
function gatewayDeposit(address dst) external payable {
require(!isZetaChain);
IGatewayEVM(gateway).deposit{value: msg.value}(dst, RevertOptions(msg.sender, false, address(0), "", 0));
}
// deposit through Gateway EVM
function gatewayDeposit(address dst) external payable {
require(!isZetaChain);
require(dst != address(0), "Invalid destination address");
IGatewayEVM(gateway).deposit{value: msg.value}(dst, RevertOptions(msg.sender, false, address(0), "", 0));
}


// deposit and call through Gateway EVM
function gatewayDepositAndCall(address dst, bytes calldata payload) external payable {
require(!isZetaChain);
IGatewayEVM(gateway).depositAndCall{value: msg.value}(dst, payload, RevertOptions(msg.sender, false, address(0), "", 0));
}
Comment on lines +178 to +181
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add validation for gatewayDepositAndCall parameters

Similar to gatewayDeposit, ensure that the dst address is validated and that the payload is not empty to prevent unintended calls.

Apply this diff to include validations:

 function gatewayDepositAndCall(address dst, bytes calldata payload) external payable {
     require(!isZetaChain);
+    require(dst != address(0), "Invalid destination address");
+    require(payload.length > 0, "Payload cannot be empty");
     IGatewayEVM(gateway).depositAndCall{value: msg.value}(dst, payload, RevertOptions(msg.sender, false, address(0), "", 0));
 }
📝 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
function gatewayDepositAndCall(address dst, bytes calldata payload) external payable {
require(!isZetaChain);
IGatewayEVM(gateway).depositAndCall{value: msg.value}(dst, payload, RevertOptions(msg.sender, false, address(0), "", 0));
}
function gatewayDepositAndCall(address dst, bytes calldata payload) external payable {
require(!isZetaChain);
require(dst != address(0), "Invalid destination address");
require(payload.length > 0, "Payload cannot be empty");
IGatewayEVM(gateway).depositAndCall{value: msg.value}(dst, payload, RevertOptions(msg.sender, false, address(0), "", 0));
}


// call through Gateway EVM
function gatewayCall(address dst, bytes calldata payload) external {
require(!isZetaChain);
IGatewayEVM(gateway).call(dst, payload, RevertOptions(msg.sender, false, address(0), "", 0));
}
Comment on lines +184 to +187
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate inputs in gatewayCall function

For robustness, validate the dst address and ensure the payload is not empty before proceeding with the call.

Apply this diff for enhanced validation:

 function gatewayCall(address dst, bytes calldata payload) external {
     require(!isZetaChain);
+    require(dst != address(0), "Invalid destination address");
+    require(payload.length > 0, "Payload cannot be empty");
     IGatewayEVM(gateway).call(dst, payload, RevertOptions(msg.sender, false, address(0), "", 0));
 }
📝 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
function gatewayCall(address dst, bytes calldata payload) external {
require(!isZetaChain);
IGatewayEVM(gateway).call(dst, payload, RevertOptions(msg.sender, false, address(0), "", 0));
}
function gatewayCall(address dst, bytes calldata payload) external {
require(!isZetaChain);
require(dst != address(0), "Invalid destination address");
require(payload.length > 0, "Payload cannot be empty");
IGatewayEVM(gateway).call(dst, payload, RevertOptions(msg.sender, false, address(0), "", 0));
}


function consumeGas() internal {
// Approximate target gas consumption
uint256 targetGas = 500000;
Expand Down
2 changes: 1 addition & 1 deletion x/crosschain/keeper/cctx_orchestrator_validate_outbound.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ func (k Keeper) processSuccessfulOutbound(
oldStatus := cctx.CctxStatus.Status
switch oldStatus {
case types.CctxStatus_PendingRevert:
cctx.SetReverted("", "revert executed")
cctx.SetReverted("", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding the value to status message instead

case types.CctxStatus_PendingOutbound:
cctx.SetOutboundMined("")
default:
Expand Down
3 changes: 2 additions & 1 deletion x/crosschain/types/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ func (m *Status) AbortRefunded() {
func (m *Status) UpdateStatusAndErrorMessages(newStatus CctxStatus, statusMsg, errorMsg string) {
m.UpdateStatus(newStatus, statusMsg)

if newStatus == CctxStatus_Aborted || newStatus == CctxStatus_Reverted || newStatus == CctxStatus_PendingRevert {
if errorMsg != "" &&
(newStatus == CctxStatus_Aborted || newStatus == CctxStatus_Reverted || newStatus == CctxStatus_PendingRevert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move status check to a function w/ a comment? It's not clear why only these statuses are eligible

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

m.UpdateErrorMessage(errorMsg)
}
Comment on lines +16 to 19
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Error message handling needs additional test coverage

Based on the test coverage analysis:

  • The status transitions are well-tested in status_test.go
  • However, there's insufficient test coverage for the new empty error message guard condition
  • No explicit test cases found for error message handling with Aborted/Reverted/PendingRevert statuses
  • Add test cases in x/crosschain/types/status_test.go to verify:
    • Empty error message is not set for Aborted status
    • Empty error message is not set for Reverted status
    • Empty error message is not set for PendingRevert status
    • Non-empty error messages are correctly set for these statuses
🔗 Analysis chain

LGTM! Verify error message handling in tests.

The additional check for non-empty error messages is a good improvement that prevents setting empty error messages. The status conditions are maintained correctly.

Let's verify the error message handling in tests:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check test coverage for error message scenarios

# Look for test cases covering error message handling
rg -A 5 "UpdateStatusAndErrorMessages.*\"\"" "**/*_test.go"

# Check for test cases with specific status transitions
ast-grep --pattern 'func Test$_(t *testing.T) {
  $$$
  UpdateStatusAndErrorMessages($_, $_, $_)
  $$$
}'

Length of output: 220

Script:

#!/bin/bash
# Let's try a different approach to find test files and error message handling

# Find test files in the crosschain directory
fd "_test.go" x/crosschain

# Look for test cases with UpdateStatusAndErrorMessages
rg -l "UpdateStatusAndErrorMessages" $(fd "_test.go" x/crosschain)

# Check the content of test files for error message scenarios
rg -A 10 "UpdateStatusAndErrorMessages" $(fd "_test.go" x/crosschain)

Length of output: 11923

Script:

#!/bin/bash
# Let's check the status_test.go file specifically for error message tests
rg -A 10 "TestStatus.*" x/crosschain/types/status_test.go

# Also check for any test cases in cctx_test.go that might test error messages
rg -A 10 "UpdateStatusAndErrorMessages|UpdateErrorMessage" x/crosschain/types/cctx_test.go

# Look for test cases with error messages in the keeper tests
rg -A 10 "UpdateStatusAndErrorMessages.*CctxStatus_(Aborted|Reverted|PendingRevert)" x/crosschain/keeper/

Length of output: 1559

}
Expand Down
7 changes: 4 additions & 3 deletions x/fungible/keeper/v2_deposits_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package keeper_test

import (
"math/big"
"testing"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/ethereum/go-ethereum/common"
"github.com/stretchr/testify/require"
Expand All @@ -11,8 +14,6 @@ import (
"github.com/zeta-chain/node/testutil/sample"
fungiblekeeper "github.com/zeta-chain/node/x/fungible/keeper"
"github.com/zeta-chain/node/x/fungible/types"
"math/big"
"testing"
)

// getTestDAppNoMessageIndex queries the no message index of the test dapp v2 contract
Expand Down Expand Up @@ -51,7 +52,7 @@ func getTestDAppNoMessageIndex(

// deployTestDAppV2 deploys the test dapp v2 contract and returns its address
func deployTestDAppV2(t *testing.T, ctx sdk.Context, k *fungiblekeeper.Keeper, evmk types.EVMKeeper) common.Address {
testDAppV2, err := k.DeployContract(ctx, testdappv2.TestDAppV2MetaData, true)
testDAppV2, err := k.DeployContract(ctx, testdappv2.TestDAppV2MetaData, true, sample.EthAddress())
require.NoError(t, err)
require.NotEmpty(t, testDAppV2)
assertContractDeployment(t, evmk, ctx, testDAppV2)
Expand Down
51 changes: 28 additions & 23 deletions zetaclient/chains/evm/observer/inbound.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,35 +145,40 @@
}
ob.Logger().Inbound.Info().Msgf("checking tracker for inbound %s chain %d", tracker.TxHash, ob.Chain().ChainId)

// if the transaction is sent to the gateway, this is a v2 inbound
// try processing the tracker for v2 inbound

Check warning on line 148 in zetaclient/chains/evm/observer/inbound.go

View check run for this annotation

Codecov / codecov/patch

zetaclient/chains/evm/observer/inbound.go#L148 

Added line #L148 was not covered by tests
gatewayAddr, gateway, err := ob.GetGatewayContract()
if err != nil {
ob.Logger().Inbound.Debug().Err(err).Msg("error getting gateway contract for processing inbound tracker")
}
if err == nil && tx != nil && ethcommon.HexToAddress(tx.To) == gatewayAddr {
if err := ob.ProcessInboundTrackerV2(ctx, gateway, tx, receipt); err != nil {
if err == nil && tx != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can tx be nil? If no, let's remove this check, otherwise tx == nil case should be handled above

Copy link

Choose a reason for hiding this comment

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

I've double-checked it and it is validated in TransactionByHash() by calling `ValidateEvmTransaction():

node/zetaclient/chains/evm/validation.go

Lines 34 to 37 in 4cd508d

func ValidateEvmTransaction(tx *ethrpc.Transaction) error {
if tx == nil {
return fmt.Errorf("transaction is nil")
}

This check here can be safely removed.

// filter error if event is not found, in this case we run v1 tracker process
if err := ob.ProcessInboundTrackerV2(ctx, gateway, gatewayAddr, tx, receipt); err != nil &&
!errors.Is(err, errEventNotFound) {

Check warning on line 156 in zetaclient/chains/evm/observer/inbound.go

View check run for this annotation

Codecov / codecov/patch

zetaclient/chains/evm/observer/inbound.go#L153-L156 

Added lines #L153 - L156 were not covered by tests
return err
} else if err == nil {
// continue with next tracker
continue

Check warning on line 160 in zetaclient/chains/evm/observer/inbound.go

View check run for this annotation

Codecov / codecov/patch

zetaclient/chains/evm/observer/inbound.go#L158-L160 

Added lines #L158 - L160 were not covered by tests
}
} else {
// check and vote on inbound tx
switch tracker.CoinType {
case coin.CoinType_Zeta:
_, err = ob.CheckAndVoteInboundTokenZeta(ctx, tx, receipt, true)
case coin.CoinType_ERC20:
_, err = ob.CheckAndVoteInboundTokenERC20(ctx, tx, receipt, true)
case coin.CoinType_Gas:
_, err = ob.CheckAndVoteInboundTokenGas(ctx, tx, receipt, true)
default:
return fmt.Errorf(
"unknown coin type %s for inbound %s chain %d",
tracker.CoinType,
tx.Hash,
ob.Chain().ChainId,
)
}
if err != nil {
return errors.Wrapf(err, "error checking and voting for inbound %s chain %d", tx.Hash, ob.Chain().ChainId)
}
}
Comment on lines +153 to +162
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid variable shadowing to enhance code clarity

The variable err is being redeclared in the inner if statement, which shadows the outer err. This can lead to confusion and potential errors. It's recommended to rename the inner err variable or restructure the code to prevent shadowing.

Apply this diff to prevent variable shadowing:

 if err == nil && tx != nil {
 	// filter error if event is not found, in this case we run v1 tracker process
-	if err := ob.ProcessInboundTrackerV2(ctx, gateway, gatewayAddr, tx, receipt); err != nil &&
+	processErr := ob.ProcessInboundTrackerV2(ctx, gateway, gatewayAddr, tx, receipt)
+	if processErr != nil && !errors.Is(processErr, errEventNotFound) {
 		return processErr
-	} else if err == nil {
+	} else if processErr == nil {
 		// continue with next tracker
 		continue
 	}
 }
📝 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
if err == nil && tx != nil {
// filter error if event is not found, in this case we run v1 tracker process
if err := ob.ProcessInboundTrackerV2(ctx, gateway, gatewayAddr, tx, receipt); err != nil &&
!errors.Is(err, errEventNotFound) {
return err
} else if err == nil {
// continue with next tracker
continue
}
} else {
// check and vote on inbound tx
switch tracker.CoinType {
case coin.CoinType_Zeta:
_, err = ob.CheckAndVoteInboundTokenZeta(ctx, tx, receipt, true)
case coin.CoinType_ERC20:
_, err = ob.CheckAndVoteInboundTokenERC20(ctx, tx, receipt, true)
case coin.CoinType_Gas:
_, err = ob.CheckAndVoteInboundTokenGas(ctx, tx, receipt, true)
default:
return fmt.Errorf(
"unknown coin type %s for inbound %s chain %d",
tracker.CoinType,
tx.Hash,
ob.Chain().ChainId,
)
}
if err != nil {
return errors.Wrapf(err, "error checking and voting for inbound %s chain %d", tx.Hash, ob.Chain().ChainId)
}
}
if err == nil && tx != nil {
// filter error if event is not found, in this case we run v1 tracker process
processErr := ob.ProcessInboundTrackerV2(ctx, gateway, gatewayAddr, tx, receipt)
if processErr != nil && !errors.Is(processErr, errEventNotFound) {
return processErr
} else if processErr == nil {
// continue with next tracker
continue
}
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 153-156: zetaclient/chains/evm/observer/inbound.go#L153-L156
Added lines #L153 - L156 were not covered by tests

[warning] 158-160: zetaclient/chains/evm/observer/inbound.go#L158-L160
Added lines #L158 - L160 were not covered by tests


// try processing the tracker for v1 inbound
switch tracker.CoinType {
case coin.CoinType_Zeta:
_, err = ob.CheckAndVoteInboundTokenZeta(ctx, tx, receipt, true)
case coin.CoinType_ERC20:
_, err = ob.CheckAndVoteInboundTokenERC20(ctx, tx, receipt, true)
case coin.CoinType_Gas:
_, err = ob.CheckAndVoteInboundTokenGas(ctx, tx, receipt, true)
default:
return fmt.Errorf(
"unknown coin type %s for inbound %s chain %d",
tracker.CoinType,
tx.Hash,
ob.Chain().ChainId,
)

Check warning on line 178 in zetaclient/chains/evm/observer/inbound.go

View check run for this annotation

Codecov / codecov/patch

zetaclient/chains/evm/observer/inbound.go#L165-L178 

Added lines #L165 - L178 were not covered by tests
}
if err != nil {
return errors.Wrapf(err, "error checking and voting for inbound %s chain %d", tx.Hash, ob.Chain().ChainId)

Check warning on line 181 in zetaclient/chains/evm/observer/inbound.go

View check run for this annotation

Codecov / codecov/patch

zetaclient/chains/evm/observer/inbound.go#L180-L181 

Added lines #L180 - L181 were not covered by tests
}
}
return nil
Expand Down
9 changes: 7 additions & 2 deletions zetaclient/chains/evm/observer/v2_inbound_tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,24 @@

import (
"context"
"errors"
"fmt"

ethcommon "github.com/ethereum/go-ethereum/common"
ethtypes "github.com/ethereum/go-ethereum/core/types"
"github.com/onrik/ethrpc"
"github.com/zeta-chain/protocol-contracts/v2/pkg/gatewayevm.sol"

"github.com/zeta-chain/node/zetaclient/zetacore"
)

var errEventNotFound = errors.New("no gateway event found in inbound tracker")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var errEventNotFound = errors.New("no gateway event found in inbound tracker")
var ErrEventNotFound = errors.New("event not found")


// ProcessInboundTrackerV2 processes inbound tracker events from the gateway
func (ob *Observer) ProcessInboundTrackerV2(
ctx context.Context,
gateway *gatewayevm.GatewayEVM,
gatewayAddr ethcommon.Address,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to pass gateway and gatewayAddr into this function? It should be within *Observer

tx *ethrpc.Transaction,
receipt *ethtypes.Receipt,
) error {
Expand All @@ -28,7 +33,7 @@
}

for _, log := range receipt.Logs {
if log == nil {
if log == nil && log.Address != gatewayAddr {

Check warning on line 36 in zetaclient/chains/evm/observer/v2_inbound_tracker.go

View check run for this annotation

Codecov / codecov/patch

zetaclient/chains/evm/observer/v2_inbound_tracker.go#L36 

Added line #L36 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Critical: Fix nil pointer dereference and add test coverage

The condition if log == nil && log.Address != gatewayAddr will cause a panic due to attempting to access log.Address after confirming log is nil. This needs to be fixed and properly tested.

  • Change the condition to use logical OR:
-		if log == nil && log.Address != gatewayAddr {
+		if log == nil || log.Address != gatewayAddr {
  • Add test coverage for nil log handling in zetaclient/chains/evm/observer/inbound_test.go
🔗 Analysis chain

Critical: Fix nil pointer dereference in log check

The current condition if log == nil && log.Address != gatewayAddr will cause a panic when log is nil, as it attempts to access log.Address after checking that log is nil.

Apply this fix:

-		if log == nil && log.Address != gatewayAddr {
+		if log == nil || log.Address != gatewayAddr {

Additionally, this line needs test coverage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing tests of this condition
rg -A 5 "ProcessInboundTrackerV2.*nil.*log" test/

Length of output: 97

Script:

#!/bin/bash
# Let's first check if the file exists and see its content around the problematic line
rg -A 5 -B 5 "if log == nil && log.Address != gatewayAddr" .

# Let's also search for test files related to inbound tracker
fd "inbound.*test.*go$"

# Search for any test files containing "tracker" in their name
fd "tracker.*test.*go$"

Length of output: 2409

Script:

#!/bin/bash
# Let's check the specific EVM inbound test file for any test cases related to nil logs
rg -A 5 "nil.*log" zetaclient/chains/evm/observer/inbound_test.go

# Let's also check the v2 inbound tracker implementation for context
rg -A 10 "func.*ProcessInboundTrackerV2" zetaclient/chains/evm/observer/v2_inbound_tracker.go

Length of output: 705

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 36-36: zetaclient/chains/evm/observer/v2_inbound_tracker.go#L36
Added line #L36 was not covered by tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo?

Copy link

Choose a reason for hiding this comment

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

+1

continue
}

Expand Down Expand Up @@ -84,5 +89,5 @@
}
}

return fmt.Errorf("no gateway event found in inbound tracker %s", tx.Hash)
return errEventNotFound

Check warning on line 92 in zetaclient/chains/evm/observer/v2_inbound_tracker.go

View check run for this annotation

Codecov / codecov/patch

zetaclient/chains/evm/observer/v2_inbound_tracker.go#L92 

Added line #L92 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Add test coverage for ProcessInboundTrackerV2 error case

The error case in ProcessInboundTrackerV2 where no gateway events are found lacks test coverage. This error path should be tested to ensure robust error handling. Consider adding the following test cases:

  • Test ProcessInboundTrackerV2 with empty logs to verify errEventNotFound is returned
  • Test ProcessInboundTrackerV2 with invalid/non-matching events to verify error handling
🔗 Analysis chain

Add test coverage for the error case

While using the predefined error is good practice, this error path lacks test coverage. Consider adding a test case where no gateway events are found in the logs.

Let me search for test files and error cases to better understand the test coverage situation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for existing error case tests
rg -A 5 "errEventNotFound" test/

Length of output: 78

Script:

#!/bin/bash
# Search for test files and error cases in the entire codebase
fd ".*_test.go" | xargs rg -A 5 "errEventNotFound"

# Find the error definition to understand its usage
rg "var errEventNotFound" 

# Search for test files in the same directory as the source file
fd ".*_test.go" zetaclient/chains/evm/observer/

Length of output: 506

Script:

#!/bin/bash
# Check the v2_inbound_tracker.go file to understand the context of errEventNotFound
rg -B 5 -A 5 "errEventNotFound =" zetaclient/chains/evm/observer/v2_inbound_tracker.go

# Look for any test files that might be testing v2_inbound_tracker functionality
rg -l "v2_inbound_tracker" zetaclient/chains/evm/observer/*_test.go

# Search for any test functions related to inbound tracking in v2
rg "func.*Test.*Inbound.*V2" zetaclient/chains/evm/observer/

Length of output: 801

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 92-92: zetaclient/chains/evm/observer/v2_inbound_tracker.go#L92
Added line #L92 was not covered by tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return errEventNotFound
return errors.Wrapf(ErrEventNotFound, "inbound tracker %s", tx.Hash)

}
Loading