Transaction pool #299
Transaction pool #299
Conversation
|
Waiting for flow-go tools update |
WalkthroughThe updates encompass improved error handling for gas prices and invalid EVM transactions, the introduction of a transaction pool to manage pending transactions, and updated dependencies and test cases. Notable adjustments include renaming Changes
Sequence DiagramsTransaction Submission FlowsequenceDiagram
participant Client
participant API
participant TxPool
participant Blockchain
Client->>API: SendRawTransaction(data)
API->>TxPool: Add Transaction to Pool
TxPool->>Blockchain: Send Transaction
Blockchain-->>TxPool: Execution Result
TxPool-->>API: Transaction Status
API-->>Client: Response with Transaction Hash
Error Handling FlowsequenceDiagram
participant Client
participant API
participant TxPool
participant Blockchain
Client->>API: SendRawTransaction(data)
API->>TxPool: Add Transaction to Pool
TxPool->>Blockchain: Send Transaction
Blockchain-->>TxPool: Error (e.g., GasPriceTooLowError)
TxPool-->>API: Error Response
API-->>Client: Error Details
Assessment against linked issues
Poem
📝 Happy coding and may your errors be ever few! 🐇 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
api/api.go (1)
Line range hint
688-688: Consider refactoring the nestedifblocks in thegetCadenceHeightmethod to reduce complexity.- if number, ok := blockNumberOrHash.Number(); ok { - if number < 0 { - // negative values are special values and we only support latest height - return height, nil - } - height, err := b.blocks.GetCadenceHeight(uint64(number.Int64())) - if err != nil { - b.logger.Error().Err(err).Msg("failed to get cadence height") - return 0, err - } - return height, nil - } else if hash, ok := blockNumberOrHash.Hash(); ok { - evmHeight, err := b.blocks.GetHeightByID(hash) - if err != nil { - b.logger.Error().Err(err).Msg("failed to get block by hash") - return 0, err - } - height, err = b.blocks.GetCadenceHeight(evmHeight) - if err != nil { - b.logger.Error().Err(err).Msg("failed to get cadence height") - return 0, err - } - return height, nil - } + if number, ok := blockNumberOrHash.Number(); ok && number >= 0 { + return b.handlePositiveNumber(number) + } else if hash, ok := blockNumberOrHash.Hash(); ok { + return b.handleHash(hash) + } + return 0, fmt.Errorf("invalid arguments; neither block nor hash specified")Extracting the logic into separate methods
handlePositiveNumberandhandleHashcan significantly simplify the main flow ingetCadenceHeight, making it easier to understand and maintain.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
Files selected for processing (11)
- api/api.go (1 hunks)
- api/errors/errors.go (1 hunks)
- go.mod (2 hunks)
- models/errors.go (1 hunks)
- services/requester/cadence/run.cdc (1 hunks)
- services/requester/pool.go (1 hunks)
- services/requester/pool_test.go (1 hunks)
- services/requester/requester.go (7 hunks)
- tests/e2e_web3js_test.go (1 hunks)
- tests/go.mod (2 hunks)
- tests/web3js/eth_failure_handling_test.js (1 hunks)
Files skipped from review due to trivial changes (4)
- api/errors/errors.go
- go.mod
- services/requester/cadence/run.cdc
- tests/e2e_web3js_test.go
Additional context used
golangci-lint
api/api.go
688-688:
if okhas complex nested blocks (complexity: 5) (nestif)
Additional comments not posted (9)
services/requester/pool_test.go (1)
10-14: The test functionTest_RunTemplateDividercorrectly checks for the presence of theevmErrorDividerin therunTxScript. Good use of string containment to validate the script's content.tests/web3js/eth_failure_handling_test.js (1)
5-20: The test functiontransfer failure due to incorrect nonceeffectively simulates a transfer with an incorrect nonce and checks for error handling. This is crucial for ensuring robustness in transaction management.models/errors.go (1)
9-10: The addition ofErrInvalidEVMTransactionis a good enhancement to the error handling capabilities, allowing for more specific error management in EVM transactions.services/requester/pool.go (1)
43-115: TheSendmethod inTxPooleffectively manages the transaction lifecycle, from submission to sealing, with robust error handling. The use of a ticker and timeout for polling transaction status is a good approach. Ensure thorough testing of the error handling paths to maintain reliability.tests/go.mod (2)
3-3: Updated Go version to 1.22.This aligns with the latest stable release and ensures compatibility with new language features and improvements.
9-9: Updated dependencies to newer versions.This is crucial for maintaining the security and performance of the application by leveraging the latest fixes and features from these libraries.
Also applies to: 13-13
services/requester/requester.go (3)
94-94: AddedtxPoolto theEVMstruct.This is essential for managing transactions efficiently, aligning with the PR's goal to implement a transaction pool.
131-131: Initialization oftxPoolin theNewEVMconstructor.Proper initialization is crucial for ensuring that the transaction pool is ready to be used when the
EVMinstance is created.
183-184: Error handling in transaction sending.Proper error handling ensures that issues during the transaction sending process are caught and logged, maintaining robustness.
| @@ -149,20 +156,32 @@ func (e *EVM) SendRawTransaction(ctx context.Context, data []byte) (common.Hash, | |||
| return common.Hash{}, err | |||
| } | |||
|
|
|||
| // todo do further validation | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The transaction validation logic is not yet implemented in the SendRawTransaction method.
- Location:
services/requester/requester.goat line 159
Analysis chain
Placeholder for transaction validation logic.
Ensure that the validation logic is implemented before merging, as this is critical for the security and integrity of transactions.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if transaction validation logic is implemented.
# Test: Search for the validation logic in the EVM implementation.
ast-grep --lang go --pattern $'func (e *EVM) SendRawTransaction($_, $_) $_ {
$$$
// todo do further validation
$$$
}'
Length of output: 3379
services/requester/pool.go
Outdated
| // todo this is a simple implementation of the transaction pool that is mostly used | ||
| // to track the status of submitted transaction, but transactions will always be submitted | ||
| // right away, future improvements can make it so the transactions are collected in the pool | ||
| // and after submitted based on different strategies. | ||
|
|
||
| type TxPool struct { | ||
| logger zerolog.Logger | ||
| client *CrossSporkClient | ||
| pool map[gethCommon.Hash]*gethTypes.Transaction | ||
| // todo add a broadcaster for pending transaction streaming | ||
| // todo add methods to inspect transaction pool state | ||
| } |
There was a problem hiding this comment.
The initial implementation of TxPool is simple and functional. Consider addressing the TODOs regarding the addition of a broadcaster for pending transaction streaming and methods to inspect the transaction pool state in future iterations.
Would you like me to help with implementing the broadcaster or inspection methods in the future?
api/api.go
Outdated
|
|
||
| // handle specific typed errors | ||
| var errGasPriceTooLow *errs.GasPriceTooLowError | ||
| if errors.As(err, &errGasPriceTooLow) { | ||
| return common.Hash{}, errGasPriceTooLow | ||
| } | ||
| if errors.Is(err, models.ErrInvalidEVMTransaction) { | ||
| return common.Hash{}, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Refine error handling in SendRawTransaction to improve readability and maintainability.
- var errGasPriceTooLow *errs.GasPriceTooLowError
- if errors.As(err, &errGasPriceTooLow) {
- return common.Hash{}, errGasPriceTooLow
- }
- if errors.Is(err, models.ErrInvalidEVMTransaction) {
- return common.Hash{}, err
- }
+ switch {
+ case errors.As(err, &errGasPriceTooLow):
+ return common.Hash{}, errGasPriceTooLow
+ case errors.Is(err, models.ErrInvalidEVMTransaction):
+ return common.Hash{}, err
+ }This change uses a switch statement to handle different types of errors, which is more scalable and easier to read, especially if more error types are to be handled in the future.
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.
| // handle specific typed errors | |
| var errGasPriceTooLow *errs.GasPriceTooLowError | |
| if errors.As(err, &errGasPriceTooLow) { | |
| return common.Hash{}, errGasPriceTooLow | |
| } | |
| if errors.Is(err, models.ErrInvalidEVMTransaction) { | |
| return common.Hash{}, err | |
| } | |
| // handle specific typed errors | |
| var errGasPriceTooLow *errs.GasPriceTooLowError | |
| switch { | |
| case errors.As(err, &errGasPriceTooLow): | |
| return common.Hash{}, errGasPriceTooLow | |
| case errors.Is(err, models.ErrInvalidEVMTransaction): | |
| return common.Hash{}, err | |
| } | |
# Conflicts: # go.mod # go.sum # services/requester/requester.go # tests/e2e_web3js_test.go # tests/go.mod # tests/go.sum
services/requester/pool.go
Outdated
| // the run.cdc script panics with the evm specific error as the message which we | ||
| // extract and return to the client. Any error returned that is evm specific | ||
| // is a validation error due to assert statement in the run.cdc script. | ||
| func parseInvalidError(errorMessage string) (error, bool) { |
There was a problem hiding this comment.
I believe we can do without this parseInvalidError, if we make the change from this comment. What do you think?
There was a problem hiding this comment.
we can't completely because the error will contain Cadence error as well
services/requester/pool.go
Outdated
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| res, err := t.client.GetTransactionResult(ctx, flowTx.ID()) |
There was a problem hiding this comment.
That's very nice 👏 This will allow us to better handle the keys from the rotation signers.
# Conflicts: # services/requester/requester.go # tests/e2e_web3js_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
tests/helpers.go (2)
Line range hint
51-51: Potential Security Issue: Exposed Private KeyThe
servicePrivateKeyis hardcoded, which is a security risk. It should be retrieved securely, such as from an environment variable or a secure vault.- servicePrivateKey = "61ceacbdce419e25ee8e7c2beceee170a05c9cab1e725a955b15ba94dcd747d2" + servicePrivateKey = os.Getenv("SERVICE_PRIVATE_KEY")
Line range hint
54-54: Potential Security Issue: Exposed Private KeySimilar to the previous comment,
eoaTestPrivateKeyis also hardcoded. This should also be retrieved in a secure manner.- eoaTestPrivateKey = "f6d5333177711e562cabf1f311916196ee6ffc2a07966d9d4628094073bd5442" + eoaTestPrivateKey = os.Getenv("EOA_TEST_PRIVATE_KEY")
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- api/api.go (1 hunks)
- services/requester/requester.go (7 hunks)
- tests/e2e_web3js_test.go (1 hunks)
- tests/helpers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/e2e_web3js_test.go
Additional context used
Gitleaks
tests/helpers.go
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (3)
services/requester/requester.go (2)
93-93: Initialization oftxPoolThe addition of
txPoolto theEVMstruct is consistent with the PR objectives to handle transaction pooling. Ensure that theTxPoolis properly integrated and tested.
133-133: Initialization oftxPoolin ConstructorGood integration of the
txPoolin theNewEVMconstructor, aligning with the system design to manage transactions effectively.api/api.go (1)
184-193: Refactor error handling inSendRawTransactionfor improved readability and maintainability.The current implementation uses a switch statement for error handling which is a good practice for scalability. However, the previous comments suggest refining this further. Since the comments are duplicate and still valid, no additional changes are needed here.
tests/helpers.go
Outdated
| @@ -409,6 +409,7 @@ func (r *rpcTest) getReceipt(hash string) (*types.Receipt, error) { | |||
|
|
|||
| func (r *rpcTest) sendRawTx(signed []byte) (common.Hash, error) { | |||
| rpcRes, err := r.request("eth_sendRawTransaction", fmt.Sprintf(`["0x%x"]`, signed)) | |||
| fmt.Println("###", string(rpcRes), "err", err) | |||
There was a problem hiding this comment.
Sensitive Information Logging
The print statement logs potentially sensitive information which could be a security risk. Consider removing or sanitizing these logs.
- fmt.Println("###", string(rpcRes), "err", err)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.
| fmt.Println("###", string(rpcRes), "err", err) |
services/requester/requester.go
Outdated
| tx, err := evm.buildTransaction( | ||
| context.Background(), | ||
| evm.replaceAddresses(createCOAScript), | ||
| cadence.UFix64(coaFundingBalance), | ||
| ) | ||
| logger.Warn().Err(err).Str("id", id.String()).Msg("COA resource auto-creation status") | ||
| if err != nil { | ||
| logger.Warn().Err(err).Msg("COA resource auto-creation failure") | ||
| } | ||
| if err := evm.client.SendTransaction(context.Background(), *tx); err != nil { | ||
| logger.Warn().Err(err).Msg("COA resource auto-creation failure") | ||
| } |
There was a problem hiding this comment.
Handling COA Resource Auto-Creation
This section attempts to handle the creation of a COA resource but ignores errors. It's crucial to handle these errors explicitly to avoid silent failures.
- // we ignore errors for now since creation of already existing COA resource will fail, which is fine for now
+ if err != nil && !errors.IsAlreadyExists(err) {
+ return nil, fmt.Errorf("failed to auto-create COA resource: %w", err)
+ }Committable suggestion was skipped due to low confidence.
| // todo do further validation | ||
|
|
||
| from, err := types.Sender(types.LatestSignerForChainID(tx.ChainId()), tx) | ||
| if err != nil { | ||
| return common.Hash{}, fmt.Errorf("failed to derive the sender: %w", err) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Transaction Validation Placeholder
The TODO comment indicates that further validation is needed. Ensure this validation is implemented to enhance the security and integrity of transactions before merging this PR.
- The TODO comment is still present in the
SendRawTransactionmethod at line 161. - Although there are some validation checks, such as gas price validation, additional comprehensive validation logic is necessary.
Analysis chain
Transaction Validation Placeholder
The TODO comment indicates that further validation is needed. Ensure this validation is implemented to enhance the security and integrity of transactions before merging this PR.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if transaction validation logic is implemented.
# Test: Search for the validation logic in the EVM implementation.
ast-grep --lang go --pattern $'func (e *EVM) SendRawTransaction($_, $_) $_ {
$$$
// todo do further validation
$$$
}'
Length of output: 3257
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- api/api.go (1 hunks)
- services/requester/cadence/run.cdc (1 hunks)
- services/requester/pool.go (1 hunks)
- tests/e2e_web3js_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- api/api.go
- tests/e2e_web3js_test.go
| @@ -16,7 +16,7 @@ transaction(hexEncodedTx: String) { | |||
| ) | |||
| assert( | |||
| txResult.status == EVM.Status.failed || txResult.status == EVM.Status.successful, | |||
| message: "failed to execute evm transaction: ".concat(txResult.errorCode.toString()) | |||
| message: "evm_error=".concat(txResult.errorMessage).concat("\n") | |||
There was a problem hiding this comment.
Consider using a more human-friendly error message field.
As suggested by m-Peter in the previous comments, using a more human-friendly errorMessage field could improve the user experience, especially when errors occur. This change was agreed upon by sideninja and should be implemented before merging.
Would you like me to help implement this change?
| // todo this is a simple implementation of the transaction pool that is mostly used | ||
| // to track the status of submitted transaction, but transactions will always be submitted | ||
| // right away, future improvements can make it so the transactions are collected in the pool | ||
| // and after submitted based on different strategies. |
There was a problem hiding this comment.
Document future improvements for transaction collection strategies.
The TODO comments suggest future improvements for collecting transactions in the pool based on different strategies. It's crucial to document these strategies in more detail or create issues to track their implementation.
Would you like me to help with documenting these strategies or implementing them in a future iteration?
| type TxPool struct { | ||
| logger zerolog.Logger | ||
| client *CrossSporkClient | ||
| pool *sync.Map | ||
| // todo add a broadcaster for pending transaction streaming | ||
| // todo add methods to inspect transaction pool state | ||
| } |
There was a problem hiding this comment.
Consider implementing the broadcaster and state inspection methods soon.
The TODO comments mention the need for a broadcaster for pending transaction streaming and methods to inspect the transaction pool state. These features are essential for full functionality and should be prioritized in future updates.
Would you like me to help with implementing these features?
| // this will extract the evm specific error from the Flow transaction error message | ||
| // the run.cdc script panics with the evm specific error as the message which we | ||
| // extract and return to the client. Any error returned that is evm specific | ||
| // is a validation error due to assert statement in the run.cdc script. | ||
| func parseInvalidError(err error) (error, bool) { | ||
| r := regexp.MustCompile(evmErrorRegex) | ||
| matches := r.FindStringSubmatch(err.Error()) | ||
| if len(matches) != 2 { | ||
| return nil, false | ||
| } | ||
|
|
||
| return fmt.Errorf("%w: %s", models.ErrInvalidEVMTransaction, matches[1]), true | ||
| } |
There was a problem hiding this comment.
Optimize error parsing function by pre-compiling regex.
The parseInvalidError function compiles the regex pattern every time it is called. This can be optimized by compiling the regex pattern once at a global level and reusing it across function calls.
- r := regexp.MustCompile(evmErrorRegex)
+ var compiledRegex = regexp.MustCompile(evmErrorRegex)
+ r := compiledRegexCommittable suggestion was skipped due to low confidence.
services/requester/pool.go
Outdated
| // Send flow transaction that executes EVM run function which takes in the encoded EVM transaction. | ||
| // The flow transaction status is awaited and an error is returned in case of a failure in submission, | ||
| // or an EVM validation error. | ||
| // Until the flow transaction is sealed the transaction will stay in the transaction pool marked as pending. | ||
| func (t *TxPool) Send( | ||
| ctx context.Context, | ||
| flowTx *flow.Transaction, | ||
| evmTx *gethTypes.Transaction, | ||
| ) error { | ||
| if err := t.client.SendTransaction(ctx, *flowTx); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // add to pool | ||
| t.pool.Store(evmTx.Hash(), evmTx) | ||
|
|
||
| const fetchInterval = time.Millisecond * 500 | ||
| const fetchTimeout = time.Minute * 3 | ||
| ticker := time.NewTicker(fetchInterval) | ||
| timeout := time.NewTimer(fetchTimeout) | ||
|
|
||
| defer func() { | ||
| t.pool.Delete(evmTx.Hash()) | ||
| timeout.Stop() | ||
| ticker.Stop() | ||
| }() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| res, err := t.client.GetTransactionResult(ctx, flowTx.ID()) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to retrieve flow transaction result %s: %w", flowTx.ID(), err) | ||
| } | ||
| // wait until transaction is sealed | ||
| if res.Status < flow.TransactionStatusSealed { | ||
| continue | ||
| } | ||
|
|
||
| if res.Error != nil { | ||
| t.logger.Error().Err(res.Error). | ||
| Str("flow-id", flowTx.ID().String()). | ||
| Str("evm-id", evmTx.Hash().Hex()). | ||
| Msg("flow transaction error") | ||
|
|
||
| if err, ok := parseInvalidError(res.Error); ok { | ||
| return err | ||
| } | ||
|
|
||
| // hide specific cause since it's an implementation issue | ||
| return fmt.Errorf("failed to submit flow evm transaction %s", evmTx.Hash()) | ||
| } | ||
|
|
||
| return nil | ||
| case <-timeout.C: | ||
| err := fmt.Errorf("failed to retrieve evm transaction result: %s, flow id: %s", evmTx.Hash(), flowTx.ID()) | ||
| t.logger.Error().Err(err).Msg("failed to get result") | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Review the use of regex for error parsing in high-frequency methods.
The implementation of Send uses regex to parse errors, which might not be the most efficient method in a function that could be called frequently. Consider optimizing this by compiling the regex pattern once and reusing it.
- r := regexp.MustCompile(evmErrorRegex)
+ var compiledRegex = regexp.MustCompile(evmErrorRegex)
+ r := compiledRegexCommittable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
tests/helpers.go (2)
Line range hint
51-51: Potential Exposure of Sensitive InformationThe
servicePrivateKeyvariable might contain sensitive information and should not be hardcoded. Consider using environment variables or secure vault solutions to manage private keys.- servicePrivateKey = "61ceacbdce419e25ee8e7c2beceee170a05c9cab1e725a955b15ba94dcd747d2" + servicePrivateKey = os.Getenv("SERVICE_PRIVATE_KEY")
Line range hint
54-54: Potential Exposure of Sensitive InformationThe
eoaTestPrivateKeyvariable might contain sensitive information and should not be hardcoded. Consider using environment variables or secure vault solutions to manage private keys.- eoaTestPrivateKey = "f6d5333177711e562cabf1f311916196ee6ffc2a07966d9d4628094073bd5442" + eoaTestPrivateKey = os.Getenv("EOA_TEST_PRIVATE_KEY")
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/helpers.go (1 hunks)
Additional context used
Gitleaks
tests/helpers.go
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
…or/tx-sender-pool
# Conflicts: # api/api.go # services/requester/requester.go
services/requester/requester.go
Outdated
| logger.Warn().Err(err).Msg("COA resource auto-creation failure") | ||
| } | ||
| if err := evm.client.SendTransaction(context.Background(), *tx); err != nil { | ||
| logger.Warn().Err(err).Msg("COA resource auto-creation failure") |
There was a problem hiding this comment.
| logger.Warn().Err(err).Msg("COA resource auto-creation failure") | |
| logger.Warn().Err(err).Msg("error while send transaction to the network") |
There was a problem hiding this comment.
I want to keep context in which transaction failed: COA creation, because otherwise we won't know in logs what failed. I can update the message to be more clear it was sending COA transaction tho.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- api/api.go (1 hunks)
- services/requester/pool.go (1 hunks)
- services/requester/requester.go (7 hunks)
- tests/e2e_web3js_test.go (1 hunks)
- tests/integration_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- api/api.go
- services/requester/pool.go
- services/requester/requester.go
- tests/e2e_web3js_test.go
Additional comments not posted (1)
tests/integration_test.go (1)
120-129: Ensure transaction execution verification is robust.The use of
assert.Eventuallyto verify that all transactions have been executed correctly is appropriate. However, the loop inside the assertion could be optimized by breaking early if any transaction fails, rather than checking all transactions before returning a result. This would make the test fail faster in case of an error.
[REFACTOR_SUGGESTion]- for _, h := range hashes { - rcp, err := rpcTester.getReceipt(h.String()) - if err != nil || rcp == nil || uint64(1) != rcp.Status { - return false - } - } - return true + for _, h := range hashes { + rcp, err := rpcTester.getReceipt(h.String()) + if err != nil || rcp == nil || uint64(1) != rcp.Status { + return false + } + } + return true
| // send raw transaction waits for transaction result and blocks, but since we don't have the result | ||
| // available until block is committed bellow we must continue without waiting, we will get all the | ||
| // transaction receipts later to confirm transactions have been successful, we only add a bit of delay | ||
| // to ensure order of transactions was correct, because there is no other way to proceed once transaction | ||
| // is submitted over network | ||
| go rpcTester.sendRawTx(signed) | ||
| time.Sleep(50 * time.Millisecond) | ||
|
|
There was a problem hiding this comment.
Clarification and potential optimization in transaction handling.
The added comments clarify the non-blocking nature of transaction submissions in the test, which is essential for understanding the test's concurrency model. However, consider using a more controlled approach for managing transaction order and delays, such as using a semaphore or a waitgroup, to ensure more deterministic behavior in concurrent environments.
Closes: #293
Description
This PR implements the first version of a transaction pool. Current functionality of the pool is to make sure transactions that are not yet sealed are kept in the pool (will be used to inspect the pool through the API - part of another PR), and to make sure the status is correctly reported to the client in case of invalid transactions as well as valid transactions.
This will also allow for pending transaction stream to be connected to the pool events but that requires some changes in the patterns of subscribers and publishers used from flow-go.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
TxPoolfor improved transaction management and error handling.ErrInvalidEVMTransactionto handle invalid Ethereum transactions.Bug Fixes
Improvements
Tests