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

Add opdevnet to integrations package #243

Merged
merged 13 commits into from
Oct 12, 2024
Merged

Add opdevnet to integrations package #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2671044
Fix duplicate error strings being printed
joshklop Oct 4, 2024
ec52fdd
Add devnet options to start command
joshklop Oct 2, 2024
c64501a
Fix dummy signer
joshklop Oct 5, 2024
6d104a7
test fixes for signer
joshklop Oct 8, 2024
7f0e6c7
Delete adapters tests
joshklop Oct 8, 2024
0f7ee67
monogen fixes
joshklop Oct 9, 2024
01b7a44
more gas fixes
joshklop Oct 9, 2024
d2c50b0
signer fixes
joshklop Oct 10, 2024
c36d1c4
new tx type
joshklop Oct 11, 2024
0b5b04f
working now
joshklop Oct 12, 2024
01acddb
remove println
joshklop Oct 12, 2024
82db06a
remove unused flags
joshklop Oct 12, 2024
3ca6bb6
remove commented out antehandler
joshklop Oct 12, 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
11 changes: 7 additions & 4 deletions .golangci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ linters-settings:
- condition
- return
ignored-numbers:
- '0'
- '1'
- '2'
- '3'
- "0"
- "1"
- "2"
- "3"
ignored-functions:
- strings.SplitN

Expand Down Expand Up @@ -123,9 +123,12 @@ issues:
- linters:
- lll
source: "^//go:generate "
exclude-files:
- opdevnet/secrets.go
exclude-dirs:
- app # This code is left over and will be changed soon.
- eth/internal/ethapi
- x/rollup/tx/internal

run:
timeout: 5m
49 changes: 15 additions & 34 deletions adapters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,21 @@ import (
"fmt"

bfttypes "github.com/cometbft/cometbft/types"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdktx "github.com/cosmos/cosmos-sdk/types/tx"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
sdktypes "github.com/cosmos/cosmos-sdk/types"
"github.com/ethereum/go-ethereum/common/hexutil"
ethtypes "github.com/ethereum/go-ethereum/core/types"
rolluptypes "github.com/polymerdao/monomer/x/rollup/types"
)

var PrivKey = secp256k1.GenPrivKeyFromSecret([]byte("monomer"))

Comment on lines +15 to +16
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

Security Concern: Hardcoded Private Key

Including a hardcoded private key in the code is a significant security risk. Hardcoding secrets can lead to vulnerabilities if the code is exposed. Consider using a secure method to manage private keys, such as environment variables, configuration files, or a secrets management system.

var errL1AttributesNotFound = errors.New("L1 attributes tx not found")

type TxSigner func(tx *sdktx.Tx) error
type TxSigner func([]sdktypes.Msg) (bfttypes.Tx, error)

// AdaptPayloadTxsToCosmosTxs assumes the deposit transactions come first.
func AdaptPayloadTxsToCosmosTxs(ethTxs []hexutil.Bytes, signTx TxSigner, from string) (bfttypes.Txs, error) {
func AdaptPayloadTxsToCosmosTxs(ethTxs []hexutil.Bytes, _ TxSigner, _ string) (bfttypes.Txs, error) {
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

Refactor Function Signature to Remove Unused Parameters

The function AdaptPayloadTxsToCosmosTxs has unused parameters _ TxSigner and _ string. Since these parameters are no longer used, consider removing them from the function signature to improve code clarity.

Apply this diff to remove the unused parameters:

-func AdaptPayloadTxsToCosmosTxs(ethTxs []hexutil.Bytes, _ TxSigner, _ string) (bfttypes.Txs, error) {
+func AdaptPayloadTxsToCosmosTxs(ethTxs []hexutil.Bytes) (bfttypes.Txs, error) {
📝 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
func AdaptPayloadTxsToCosmosTxs(ethTxs []hexutil.Bytes, _ TxSigner, _ string) (bfttypes.Txs, error) {
func AdaptPayloadTxsToCosmosTxs(ethTxs []hexutil.Bytes) (bfttypes.Txs, error) {

if len(ethTxs) == 0 {
return bfttypes.Txs{}, nil
}
Expand All @@ -27,24 +29,17 @@ func AdaptPayloadTxsToCosmosTxs(ethTxs []hexutil.Bytes, signTx TxSigner, from st
return nil, fmt.Errorf("count deposit transactions: %v", err)
}

depositTx, err := packDepositTxsToCosmosTx(ethTxs[:numDepositTxs], from)
depositTx, err := packDepositTxsToCosmosTx(ethTxs[:numDepositTxs], "")
if err != nil {
return nil, fmt.Errorf("pack deposit txs: %v", err)
}

if signTx != nil {
if err := signTx(depositTx); err != nil {
return nil, fmt.Errorf("sign tx: %v", err)
}
}

depositSDKMsgBytes, err := depositTx.Marshal()
depositTxBytes, err := depositTx.Marshal()
if err != nil {
return nil, fmt.Errorf("marshal tx: %v", err)
}

cosmosTxs := make(bfttypes.Txs, 0, 1+numDepositTxs)
cosmosTxs = append(cosmosTxs, depositSDKMsgBytes)
cosmosTxs = append(cosmosTxs, depositTxBytes)

cosmosNonDepositTxs, err := convertToCosmosNonDepositTxs(ethTxs[numDepositTxs:])
if err != nil {
Expand Down Expand Up @@ -76,20 +71,14 @@ func countDepositTransactions(ethTxs []hexutil.Bytes) (int, error) {
return numDepositTxs, nil
}

func packDepositTxsToCosmosTx(depositTxs []hexutil.Bytes, from string) (*sdktx.Tx, error) {
func packDepositTxsToCosmosTx(depositTxs []hexutil.Bytes, _ string) (*rolluptypes.MsgApplyL1Txs, error) { //nolint:unparam
depositTxsBytes := make([][]byte, 0, len(depositTxs))
for _, depositTx := range depositTxs {
depositTxsBytes = append(depositTxsBytes, depositTx)
}
msgAny, err := codectypes.NewAnyWithValue(&rolluptypes.MsgApplyL1Txs{
TxBytes: depositTxsBytes,
FromAddress: from,
})
if err != nil {
return nil, fmt.Errorf("new any with value: %v", err)
}

return &sdktx.Tx{Body: &sdktx.TxBody{Messages: []*codectypes.Any{msgAny}}}, nil
return &rolluptypes.MsgApplyL1Txs{
TxBytes: depositTxsBytes,
}, nil
Comment on lines +74 to +81
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

Remove Unused Parameter from packDepositTxsToCosmosTx Function

The parameter _ string in the function packDepositTxsToCosmosTx is unused. To simplify the function signature, consider removing this parameter.

Apply this diff to remove the unused parameter:

-func packDepositTxsToCosmosTx(depositTxs []hexutil.Bytes, _ string) (*rolluptypes.MsgApplyL1Txs, error) { //nolint:unparam
+func packDepositTxsToCosmosTx(depositTxs []hexutil.Bytes) (*rolluptypes.MsgApplyL1Txs, error) { //nolint:unparam

And update the function call accordingly at line 32:

-	depositTx, err := packDepositTxsToCosmosTx(ethTxs[:numDepositTxs], "")
+	depositTx, err := packDepositTxsToCosmosTx(ethTxs[:numDepositTxs])

Committable suggestion was skipped due to low confidence.

}

func convertToCosmosNonDepositTxs(nonDepositTxs []hexutil.Bytes) (bfttypes.Txs, error) {
Expand Down Expand Up @@ -123,17 +112,9 @@ func AdaptCosmosTxsToEthTxs(cosmosTxs bfttypes.Txs) (ethtypes.Transactions, erro
}

func GetDepositTxs(txsBytes [][]byte) (ethtypes.Transactions, error) {
cosmosEthTx := new(sdktx.Tx)
if err := cosmosEthTx.Unmarshal(txsBytes[0]); err != nil {
return nil, fmt.Errorf("unmarshal cosmos tx: %v", err)
}
msgs := cosmosEthTx.GetBody().GetMessages()
if num := len(msgs); num != 1 {
return nil, fmt.Errorf("unexpected number of msgs in Eth Cosmos tx: want 1, got %d", num)
}
msg := new(rolluptypes.MsgApplyL1Txs)
if err := msg.Unmarshal(msgs[0].GetValue()); err != nil {
return nil, fmt.Errorf("unmarshal MsgL1Txs smsg: %v", err)
if err := msg.Unmarshal(txsBytes[0]); err != nil {
return nil, fmt.Errorf("unmarshal MsgL1Txs msg: %v", err)
Comment on lines +116 to +117
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

Potential Index Out of Range Error

The code accesses txsBytes[0] without verifying that txsBytes is not empty, which could lead to a runtime panic if txsBytes is empty.

Consider adding a check to ensure that txsBytes contains at least one element before accessing txsBytes[0]. For example:

if len(txsBytes) == 0 {
	return nil, errors.New("no transactions provided")
}
if err := msg.Unmarshal(txsBytes[0]); err != nil {
	return nil, fmt.Errorf("unmarshal MsgL1Txs msg: %v", err)
}

}
ethTxsBytes := msg.GetTxBytes()
if len(ethTxsBytes) == 0 {
Expand Down
230 changes: 0 additions & 230 deletions adapters_test.go
View file
Open in desktop

This file was deleted.

5 changes: 4 additions & 1 deletion builder/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,10 @@ func (b *Builder) parseWithdrawalMessages(
if execTxResult.IsOK() {
cosmosTx := new(sdktx.Tx)
if err := cosmosTx.Unmarshal(tx); err != nil {
return nil, fmt.Errorf("unmarshal cosmos tx: %v", err)
// TODO we should check for withdrawal messages on all transaction types.
// Unfortunately, we can't get the app's TxDecoder inside the builder.
// We may want to explore using a PostHandler to manage withdrawals instead.
return execTxResult, nil
Comment on lines +287 to +290
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

Consider the implications of ignoring unmarshalling errors

The changes to error handling in parseWithdrawalMessages raise some concerns:

  1. Ignoring unmarshalling errors could lead to missed withdrawal messages, potentially affecting the system's integrity.
  2. The TODO comments suggest that the current implementation is not ideal and needs improvement.

Consider the following recommendations:

  1. Instead of silently returning on unmarshalling errors, log these errors for debugging purposes.
  2. Implement a more robust solution for checking withdrawal messages across all transaction types.
  3. Explore the suggested PostHandler approach for managing withdrawals, as it might provide a more comprehensive solution.

To address these concerns, consider implementing a logging mechanism for unmarshalling errors:

 if err := cosmosTx.Unmarshal(tx); err != nil {
+    // Log the error for debugging purposes
+    b.logger.Error("Failed to unmarshal transaction", "error", err)
     // TODO we should check for withdrawal messages on all transaction types.
     // Unfortunately, we can't get the app's TxDecoder inside the builder.
     // We may want to explore using a PostHandler to manage withdrawals instead.
     return execTxResult, nil
 }

Additionally, create a task to explore the PostHandler approach for managing withdrawals. This could potentially resolve the issues with checking withdrawal messages across all transaction types.

Committable suggestion was skipped due to low confidence.

}
for _, msg := range cosmosTx.GetBody().GetMessages() {
withdrawalMsg := new(rolluptypes.MsgInitiateWithdrawal)
Expand Down
2 changes: 1 addition & 1 deletion e2e/url/url.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func Parse(stdURL *url.URL) (*URL, error) {
}

if !stdURL.IsAbs() {
return nil, fmt.Errorf("url is not absolute (does not contain scheme) %s", stdURL)
return nil, fmt.Errorf("url is not absolute (does not contain scheme): %q", stdURL)
}

// Make a deep copy of the url.
Expand Down
5 changes: 1 addition & 4 deletions engine/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (

abci "github.com/cometbft/cometbft/abci/types"
appchainClient "github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
"github.com/ethereum-optimism/optimism/op-service/eth"
"github.com/ethereum/go-ethereum/beacon/engine"
"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -50,11 +49,9 @@ func NewEngineAPI(
appchainCtx *appchainClient.Context,
metrics Metrics,
) *EngineAPI {
privKey := ed25519.GenPrivKeyFromSecret([]byte("monomer")) // TODO: configurable

return &EngineAPI{
txValidator: txValidator,
signer: signer.New(appchainCtx, privKey),
signer: signer.New(appchainCtx, monomer.PrivKey),
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

Potential security issue: Usage of global monomer.PrivKey

Using monomer.PrivKey directly when initializing the signer may pose security risks, as global variables can be accessed or modified unintentionally from other parts of the codebase. It's advisable to securely pass the private key as a parameter or retrieve it from a secure source to ensure proper key management.

Consider updating the code to pass the private key explicitly:

 func NewEngineAPI(
     b *builder.Builder,
     txValidator TxValidator,
     blockStore DB,
     appchainCtx *appchainClient.Context,
     metrics Metrics,
+    privKey someKeyType,
 ) *EngineAPI {
     return &EngineAPI{
         txValidator: txValidator,
-        signer:      signer.New(appchainCtx, monomer.PrivKey),
+        signer:      signer.New(appchainCtx, privKey),
         blockStore:  blockStore,
         builder:     b,
         metrics:     metrics,
     }
 }

Committable suggestion was skipped due to low confidence.

blockStore: blockStore,
builder: b,
metrics: metrics,
Expand Down
Loading
Loading