Skip to content
This repository has been archived by the owner on Jun 9, 2024. It is now read-only.

fix(miner): Use pending block and add nil check. #1262

Merged
merged 8 commits into from
Oct 31, 2023
Merged

fix(miner): Use pending block and add nil check. #1262

merged 8 commits into from
Oct 31, 2023

Conversation

itsdevbear
Copy link
Member

@itsdevbear itsdevbear commented Oct 31, 2023

Solves RPC error on non-validator nodes.

Summary by CodeRabbit

Refactor:

  • Restructured the build system and Docker configuration for improved efficiency and maintainability.
  • Updated the output directory path for the build process and modified Docker container execution commands.
  • Enhanced the code generation process by adding new tools (abigen, moq, mockery).

Tests:

  • Improved unit testing with the inclusion of forge-test for enhanced test coverage.

Chores:

  • Enhanced linting with the addition of forge-lint and gosec for better code quality.
  • Updated dependency management with go work sync for improved dependency synchronization.

Note: These changes primarily affect developers and do not directly impact end-users.

Copy link

coderabbitai bot commented Oct 31, 2023

Walkthrough

The changes primarily focus on restructuring the build system and Docker configuration, modifying the output directory paths, and updating the scripts for proto generation, testing, and linting. The modifications also include changes in the handling of pending blocks in the Ethereum Polar API backend.

Changes

File(s) Summary
Makefile Significant restructuring of the build system and Docker configuration, with changes to various build targets and introduction of a new main.mk file.
build/scripts/cosmos.mk, build/scripts/main.mk Changes to the build system, Docker configuration, code generation, testing, linting, and dependency management.
build/scripts/proto_generate.sh Modification in the command used to run a Docker container, indicating a change in the location or name of the script being executed.
e2e/precompile/polard/start-node.sh, e2e/testapp/docker/local/docker-init.sh, e2e/testapp/entrypoint.sh Changes in configuration files and scripts, primarily involving updates to the home directory for the polard instance.
eth/polar/api_backend.go Modifications in the handling of pending blocks in the Ethereum Polar API backend.

🐇🍂

As the leaves fall and the air grows cold,

In the world of code, we're bold and bold.

Changes made, scripts updated, paths renewed,

In the spirit of Halloween, no fright ensued.

Docker's ghost and the build system's ghoul,

Dance together in the autumn cool.

Celebrate the changes, for they're a treat,

In the world of code, we never admit defeat! 🎃👻


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 20

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 9e9c64f and 315f05d.
Files ignored due to filter (3)
  • build/tools/go.mod
  • build/tools/go.sum
  • go.work
Files selected for processing (8)
  • Makefile (1 hunks)
  • build/scripts/cosmos.mk (1 hunks)
  • build/scripts/main.mk (1 hunks)
  • build/scripts/proto_generate.sh (2 hunks)
  • e2e/precompile/polard/start-node.sh (2 hunks)
  • e2e/testapp/docker/local/docker-init.sh (2 hunks)
  • e2e/testapp/entrypoint.sh (3 hunks)
  • eth/polar/api_backend.go (3 hunks)
Files skipped from review due to trivial changes (3)
  • build/scripts/cosmos.mk
  • e2e/precompile/polard/start-node.sh
  • e2e/testapp/docker/local/docker-init.sh
Additional comments: 11
Makefile (1)
  • 1-2: The inclusion of main.mk is a good practice for separating build scripts from the main Makefile, improving maintainability and modularity.
build/scripts/proto_generate.sh (2)
  • 24-30: The instructions for running the script manually have been updated to reflect the new path of the protocgen.sh script. Ensure that the new path is correct and that the protocgen.sh script exists at that location.
- # docker run --rm -v $(pwd):/workspace --workdir /workspace cosmossdk-proto sh ./scripts/protocgen.sh
+ # docker run --rm -v $(pwd):/workspace --workdir /workspace cosmossdk-proto sh ./build/scripts/protocgen.sh
  • 52-52: The script proto_generate_pulsar.sh is being executed. Ensure that this script exists at the specified location and that it is executable.
- ./scripts/proto_generate_pulsar.sh
+ ./build/scripts/proto_generate_pulsar.sh
eth/polar/api_backend.go (3)
  • 194-201: The HeaderByNumber function has been updated to handle the case when the block number is rpc.PendingBlockNumber. It retrieves the pending block from the miner and returns its header. Ensure that the miner is always available and the pending block is correctly maintained.

  • 243-247: The BlockByNumber function has been updated to handle the case when the block number is rpc.PendingBlockNumber. It retrieves the pending block from the miner and returns it. Ensure that the miner is always available and the pending block is correctly maintained.

  • 307-316: The StateAndHeaderByNumber function has been updated to handle the case when the block number is rpc.PendingBlockNumber. It retrieves the pending block and state from the miner and returns them. Ensure that the miner is always available and the pending block and state are correctly maintained.

e2e/testapp/entrypoint.sh (4)
  • 30-36: The script now sets a dedicated home directory for the ./build/bin/polard instance. Ensure that the new directory path is correctly reflected in all parts of the codebase that reference it.

  • 64-78: The script now initializes the polard instance, sets client config, and adds keys using the ./build/bin/polard command instead of ./bin/polard. Ensure that all calls to these commands have been updated to use the new path.

  • 85-113: The script now performs genesis account operations, signs the genesis transaction, collects genesis transactions, and validates the genesis using the ./build/bin/polard command instead of ./bin/polard. Ensure that all calls to these commands have been updated to use the new path.

  • 116-116: The script now starts the polard instance using the ./build/bin/polard command instead of ./bin/polard. Ensure that all calls to this command have been updated to use the new path.

build/scripts/main.mk (1)
  • 1-2: Ensure that the included files cosmos.mk and constants.mk exist and are in the correct path.

Comment on lines +12 to +25
BUILD_TARGETS := build install

build: BUILD_ARGS=-o $(OUT_DIR)/

build-linux-amd64:
GOOS=linux GOARCH=amd64 LEDGER_ENABLED=false $(MAKE) build

build-linux-arm64:
GOOS=linux GOARCH=arm64 LEDGER_ENABLED=false $(MAKE) build

$(BUILD_TARGETS): forge-build sync $(OUT_DIR)/
@echo "Building ${TESTAPP_DIR}"
@cd ${CURRENT_DIR}/$(TESTAPP_DIR) && go $@ -mod=readonly $(BUILD_FLAGS) $(BUILD_ARGS) ./...

Copy link

Choose a reason for hiding this comment

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

The build targets are defined and used correctly. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +29 to +36
build-clean:
@$(MAKE) clean build

clean:
@rm -rf .tmp/
@rm -rf $(OUT_DIR)
@$(MAKE) forge-clean

Copy link

Choose a reason for hiding this comment

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

The build-clean and clean targets are well defined. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +41 to +45
forge-build: |
@forge build --extra-output-files bin --extra-output-files abi --root $(CONTRACTS_DIR)

forge-clean: |
@forge clean --root $(CONTRACTS_DIR)
Copy link

Choose a reason for hiding this comment

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

The forge-build and forge-clean targets are well defined. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +52 to +60
protoImageName := "ghcr.io/cosmos/proto-builder"
protoImageVersion := "0.14.0"

proto:
@$(MAKE) buf-lint-fix buf-lint proto-build

proto-build:
@docker run --rm -v ${CURRENT_DIR}:/workspace --workdir /workspace $(protoImageName):$(protoImageVersion) sh ./build/scripts/proto_generate.sh

Copy link

Choose a reason for hiding this comment

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

The proto and proto-build targets are well defined. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +65 to +116
# Variables
DOCKER_TYPE ?= base
ARCH ?= arm64
GO_VERSION ?= 1.21.3
IMAGE_NAME ?= polard
IMAGE_VERSION ?= v0.0.0
BASE_IMAGE ?= polard/base:$(IMAGE_VERSION)

# Docker Paths
BASE_DOCKER_PATH = ./e2e/testapp/docker
EXEC_DOCKER_PATH = $(BASE_DOCKER_PATH)/base.Dockerfile
LOCAL_DOCKER_PATH = $(BASE_DOCKER_PATH)/local/Dockerfile
SEED_DOCKER_PATH = $(BASE_DOCKER_PATH)/seed/Dockerfile
VAL_DOCKER_PATH = $(BASE_DOCKER_PATH)/validator/Dockerfile
LOCALNET_CLIENT_PATH = ./e2e/precompile/polard
LOCALNET_DOCKER_PATH = $(LOCALNET_CLIENT_PATH)/Dockerfile

# Image Build
docker-build:
@echo "Build a release docker image for the Cosmos SDK chain..."
@$(MAKE) docker-build-$(DOCKER_TYPE)

# Docker Build Types
docker-build-base:
$(call docker-build-helper,$(EXEC_DOCKER_PATH),base)

docker-build-local:
$(call docker-build-helper,$(LOCAL_DOCKER_PATH),local,--build-arg BASE_IMAGE=$(BASE_IMAGE))

docker-build-seed:
$(call docker-build-helper,$(SEED_DOCKER_PATH),seed,--build-arg BASE_IMAGE=$(BASE_IMAGE))

docker-build-validator:
$(call docker-build-helper,$(VAL_DOCKER_PATH),validator,--build-arg BASE_IMAGE=$(BASE_IMAGE))

docker-build-localnet:
$(call docker-build-helper,$(LOCALNET_DOCKER_PATH),localnet,--build-arg BASE_IMAGE=$(BASE_IMAGE))

# Docker Build Function
define docker-build-helper
docker build \
--build-arg GO_VERSION=$(GO_VERSION) \
--platform linux/$(ARCH) \
--build-arg PRECOMPILE_CONTRACTS_DIR=$(CONTRACTS_DIR) \
--build-arg GOOS=linux \
--build-arg GOARCH=$(ARCH) \
-f $(1) \
-t $(IMAGE_NAME)/$(2):$(IMAGE_VERSION) \
$(if $(3),$(3)) \
.

endef
Copy link

Choose a reason for hiding this comment

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

The Docker build targets and helper function are well defined. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +332 to +348
license-install:
@echo "--> Installing google/addlicense"
@go install github.com/google/addlicense

license:
@$(MAKE) license-install
@echo "--> Running addlicense with -check"
@for module in $(MODULES); do \
(cd $$module && addlicense -check -v -f ./LICENSE.header ./.) || exit 1; \
done

license-fix:
@$(MAKE) license-install
@echo "--> Running addlicense"
@for module in $(MODULES); do \
(cd $$module && addlicense -v -f ./LICENSE.header ./.) || exit 1; \
done
Copy link

Choose a reason for hiding this comment

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

The license-install, license, and license-fix targets are well defined. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +354 to +363

gosec-install:
@echo "--> Installing gosec"
@go install github.com/securego/gosec/v2/cmd/gosec

gosec:
@$(MAKE) gosec-install
@echo "--> Running gosec"
@gosec -exclude-generated ./...

Copy link

Choose a reason for hiding this comment

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

The gosec-install and gosec targets are well defined. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +368 to +384

protoDir := "proto"

buf-install:
@echo "--> Installing buf"
@go install github.com/bufbuild/buf/cmd/buf

buf-lint-fix:
@$(MAKE) buf-install
@echo "--> Running buf format"
@buf format -w --error-format=json $(protoDir)

buf-lint:
@$(MAKE) buf-install
@echo "--> Running buf lint"
@buf lint --error-format=json $(protoDir)

Copy link

Choose a reason for hiding this comment

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

The buf-install, buf-lint-fix, and buf-lint targets are well defined. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +389 to +409

sync: |
@for module in $(MODULES); do \
echo "Running go mod download in $$module"; \
(cd $$module && go mod download) || exit 1; \
done
@echo "Running go mod sync"
@go work sync

tidy: |
@for module in $(MODULES); do \
echo "Running go mod tidy in $$module"; \
(cd $$module && go mod tidy) || exit 1; \
done

repo-rinse: |
git clean -xfd
git submodule foreach --recursive git clean -xfd
git submodule foreach --recursive git reset --hard
git submodule update --init --recursive

Copy link

Choose a reason for hiding this comment

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

The sync, tidy, and repo-rinse targets are well defined. However, consider adding comments to explain what each target does for better maintainability.

Comment on lines +411 to +420
.PHONY: build build-linux-amd64 build-linux-arm64 \
$(BUILD_TARGETS) $(OUT_DIR)/ build-clean clean \
forge-build forge-clean proto proto-build docker-build \
docker-build-base docker-build-local docker-build-seed \
docker-build-validator docker-build-localnet generate \
abigen-install moq-install mockery-install mockery \
start test-unit test-unit-race test-unit-cover forge-test \
test-e2e test-e2e-no-build hive-setup hive-view test-hive \
test-hive-v test-localnet test-localnet-no-build format lint \
forge-lint-fix forge-lint golangci-install golangci golangci-fix \
Copy link

Choose a reason for hiding this comment

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

The .PHONY targets are well defined. However, consider adding comments to explain what each target does for better maintainability.

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #1262 (cca0785) into main (e2202a1) will decrease coverage by 0.34%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1262      +/-   ##
==========================================
- Coverage   49.22%   48.88%   -0.34%     
==========================================
  Files          77       78       +1     
  Lines        4642     4674      +32     
==========================================
  Hits         2285     2285              
- Misses       2195     2227      +32     
  Partials      162      162              
Files Coverage Δ
cosmos/txpool/mempool.go 48.57% <0.00%> (-1.43%) ⬇️
cosmos/txpool/ante.go 0.00% <0.00%> (ø)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 315f05d and 4715a34.
Files ignored due to filter (9)
  • build/tools/go.mod
  • build/tools/go.sum
  • contracts/foundry.toml
  • cosmos/go.mod
  • cosmos/go.sum
  • e2e/testapp/go.mod
  • e2e/testapp/go.sum
  • eth/go.mod
  • eth/go.sum
Files selected for processing (40)
  • build/scripts/main.mk (1 hunks)
  • contracts/bindings/cosmos/lib/cosmos_types.abigen.go (1 hunks)
  • contracts/bindings/testing/consume_gas.abigen.go (1 hunks)
  • contracts/bindings/testing/distribution_testing_helper.abigen.go (1 hunks)
  • contracts/bindings/testing/governance/governance_wrapper.abigen.go (1 hunks)
  • contracts/bindings/testing/liquid_staking.abigen.go (1 hunks)
  • contracts/bindings/testing/precompile_constructor.abigen.go (1 hunks)
  • contracts/bindings/testing/solmate_erc20.abigen.go (1 hunks)
  • contracts/hardhat.config.js (1 hunks)
  • contracts/src/cosmos/CosmosTypes.sol (2 hunks)
  • contracts/src/cosmos/precompile/Bank.sol (1 hunks)
  • contracts/src/cosmos/precompile/Distribution.sol (1 hunks)
  • contracts/src/cosmos/precompile/Governance.sol (1 hunks)
  • contracts/src/cosmos/precompile/Staking.sol (1 hunks)
  • contracts/src/cosmos/precompile/examples/Deploy.s.sol (1 hunks)
  • contracts/src/cosmos/precompile/examples/LiquidStaking.sol (1 hunks)
  • contracts/src/testing/ConsumeGas.sol (1 hunks)
  • contracts/src/testing/SolmateERC20.sol (1 hunks)
  • cosmos/config/mocks/app_options.go (1 hunks)
  • cosmos/runtime/ante/ante.go (1 hunks)
  • cosmos/runtime/runtime.go (2 hunks)
  • cosmos/txpool/ante.go (1 hunks)
  • cosmos/txpool/mempool.go (1 hunks)
  • cosmos/txpool/mocks/geth_tx_pool.go (3 hunks)
  • cosmos/txpool/mocks/lifecycle.go (1 hunks)
  • cosmos/txpool/mocks/sdk_tx.go (1 hunks)
  • cosmos/txpool/mocks/subscription.go (1 hunks)
  • cosmos/txpool/mocks/tx_broadcaster.go (1 hunks)
  • cosmos/txpool/mocks/tx_serializer.go (1 hunks)
  • cosmos/txpool/mocks/tx_sub_provider.go (1 hunks)
  • eth/core/state/journal/mocks/accesslist.go (1 hunks)
  • eth/core/state/journal/mocks/log.go (1 hunks)
  • eth/core/state/journal/mocks/refund.go (1 hunks)
  • eth/core/state/journal/mocks/self_destruct_state_plugin.go (1 hunks)
  • eth/core/state/journal/mocks/self_destructs.go (1 hunks)
  • eth/core/state/journal/mocks/transient_storage.go (1 hunks)
  • eth/core/state/mocks/plugin.go (1 hunks)
  • eth/core/state/mocks/polar_state_db.go (1 hunks)
  • eth/core/state/mocks/precompile_plugin.go (1 hunks)
  • eth/eth.go (2 hunks)
Files not summarized due to errors (3)
  • contracts/bindings/testing/governance/governance_wrapper.abigen.go: Error: Message exceeds token limit
  • contracts/bindings/testing/liquid_staking.abigen.go: Error: Message exceeds token limit
  • contracts/bindings/testing/solmate_erc20.abigen.go: Error: Message exceeds token limit
Files not reviewed due to errors (3)
  • contracts/bindings/testing/governance/governance_wrapper.abigen.go (Error: diff too large)
  • contracts/bindings/testing/liquid_staking.abigen.go (Error: diff too large)
  • contracts/bindings/testing/solmate_erc20.abigen.go (Error: diff too large)
Files skipped from review due to trivial changes (31)
  • contracts/bindings/cosmos/lib/cosmos_types.abigen.go
  • contracts/bindings/testing/consume_gas.abigen.go
  • contracts/bindings/testing/distribution_testing_helper.abigen.go
  • contracts/bindings/testing/precompile_constructor.abigen.go
  • contracts/hardhat.config.js
  • contracts/src/cosmos/precompile/Bank.sol
  • contracts/src/cosmos/precompile/Distribution.sol
  • contracts/src/cosmos/precompile/Governance.sol
  • contracts/src/cosmos/precompile/Staking.sol
  • contracts/src/cosmos/precompile/examples/Deploy.s.sol
  • contracts/src/cosmos/precompile/examples/LiquidStaking.sol
  • contracts/src/testing/ConsumeGas.sol
  • contracts/src/testing/SolmateERC20.sol
  • cosmos/config/mocks/app_options.go
  • cosmos/txpool/mempool.go
  • cosmos/txpool/mocks/lifecycle.go
  • cosmos/txpool/mocks/sdk_tx.go
  • cosmos/txpool/mocks/subscription.go
  • cosmos/txpool/mocks/tx_broadcaster.go
  • cosmos/txpool/mocks/tx_serializer.go
  • cosmos/txpool/mocks/tx_sub_provider.go
  • eth/core/state/journal/mocks/accesslist.go
  • eth/core/state/journal/mocks/log.go
  • eth/core/state/journal/mocks/refund.go
  • eth/core/state/journal/mocks/self_destruct_state_plugin.go
  • eth/core/state/journal/mocks/self_destructs.go
  • eth/core/state/journal/mocks/transient_storage.go
  • eth/core/state/mocks/plugin.go
  • eth/core/state/mocks/polar_state_db.go
  • eth/core/state/mocks/precompile_plugin.go
  • eth/eth.go
Additional comments: 9
cosmos/txpool/mocks/geth_tx_pool.go (3)
  • 1-13: The import paths and package names look correct. The mockery tool version has been updated.

  • 74-89: The Has function has been added to the GethTxPool interface. It seems to be correctly implemented and returns a boolean value.

  • 167-182: The Status function has been added to the GethTxPool interface. It seems to be correctly implemented and returns a coretxpool.TxStatus value.

contracts/src/cosmos/CosmosTypes.sol (1)
  • 26-26: The Solidity version has been updated. Ensure that this version is compatible with the rest of your codebase and that it doesn't introduce any breaking changes.
cosmos/runtime/runtime.go (2)
  • 33-40: The import paths seem to be correct and the imported packages are being used in the code. Ensure that these packages are included in your project dependencies.

  • 110-115: The SetAnteHandler method is being called with a new AnteHandler from the ante package. Ensure that the NewAnteHandler function is implemented correctly in the ante package and that it returns an AnteHandler that is compatible with the SetAnteHandler method.

cosmos/runtime/ante/ante.go (1)
  • 21-38: The NewAnteHandler function now accepts a mempool parameter of type *txpool.Mempool and includes it in the anteDecorators slice. Ensure that all calls to NewAnteHandler have been updated to match the new function signature. Also, verify that the mempool object is being correctly used within the sdk.ChainAnteDecorators function.
cosmos/txpool/ante.go (2)
  • 1-19: The license header is well-formatted and clear. It's good to see that the copyright and license information is included at the top of the file.

  • 23-33: The import statements are well-organized and separated by functionality, which is a good practice for readability and maintainability.

Comment on lines 64 to 73
*/
contract CosmosTypes {
function coin(Cosmos.Coin calldata) public pure {}

function pageRequest(Cosmos.PageRequest calldata) public pure {}

function pageResponse(Cosmos.PageResponse calldata) public pure {}

function codecAny(Cosmos.CodecAny calldata) public pure {}
}
Copy link

Choose a reason for hiding this comment

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

The functions coin, pageRequest, pageResponse, and codecAny don't have any implementation. If these are meant to be abstract, consider declaring this contract as abstract. If these are placeholders for future implementation, consider adding a // TODO: Implement function comment to each.

Comment on lines 54 to 61
// shouldEject returns true if the transaction should be ejected from the CometBFT mempool.
func (m *Mempool) shouldEject(tx *coretypes.Transaction) bool {
txHash := tx.Hash()
// Ejection conditions
// 1. If the transaction has been included in a block.
// TODO: we should add somemore conditons later.
return m.txpool.Status(txHash) == txpool.TxStatusIncluded
}
Copy link

Choose a reason for hiding this comment

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

The shouldEject method is well-implemented. It checks if the transaction should be ejected from the CometBFT mempool. However, the comment on line 59 indicates that more conditions should be added later. It would be good to add a TODO tag to this comment to make it more noticeable.

- // TODO: we should add somemore conditons later.
+ // TODO: Add more conditions for ejecting transactions from the CometBFT mempool.

Commitable suggestion (Beta)
Suggested change
// shouldEject returns true if the transaction should be ejected from the CometBFT mempool.
func (m *Mempool) shouldEject(tx *coretypes.Transaction) bool {
txHash := tx.Hash()
// Ejection conditions
// 1. If the transaction has been included in a block.
// TODO: we should add somemore conditons later.
return m.txpool.Status(txHash) == txpool.TxStatusIncluded
}
// shouldEject returns true if the transaction should be ejected from the CometBFT mempool.
func (m *Mempool) shouldEject(tx *coretypes.Transaction) bool {
txHash := tx.Hash()
// Ejection conditions
// 1. If the transaction has been included in a block.
// TODO: Add more conditions for ejecting transactions from the CometBFT mempool.
return m.txpool.Status(txHash) == txpool.TxStatusIncluded
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 4715a34 and cca0785.
Files selected for processing (3)
  • cosmos/runtime/runtime.go (3 hunks)
  • cosmos/txpool/ante.go (1 hunks)
  • cosmos/txpool/mempool.go (2 hunks)
Additional comments: 9
cosmos/runtime/runtime.go (3)
  • 33-40: The import paths have been updated. Ensure that the new paths are correct and the packages exist at those locations.

  • 96-98: The WrappedTxPool and WrappedMiner fields are being initialized with the txpool.New and miner.New functions respectively. Ensure that these functions return the expected types and that the arguments passed to them are correct.

  • 113-113: The SetAnteHandler function is being called with antelib.NewAnteHandler(p.WrappedTxPool) as the argument. Ensure that antelib.NewAnteHandler returns a valid sdk.AnteHandler and that p.WrappedTxPool is the correct argument to pass.

cosmos/txpool/mempool.go (3)
  • 33-39: The new import statement for the package "pkg.berachain.dev/polaris/eth/core" is introduced correctly. Ensure that the package is available and accessible.

  • 64-64: The new field "chain" of type "core.ChainReader" is added to the "Mempool" struct. This change seems to be in line with the pull request summary.

  • 69-74: The "NewMempool" function now takes an additional parameter "chain" of type "core.ChainReader". Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

- func New(txpool eth.TxPool) *Mempool {
+ func New(chain core.ChainReader, txpool eth.TxPool) *Mempool {
cosmos/txpool/ante.go (3)
  • 1-19: The license header is well-formatted and clear. It's good to see that the copyright and license information is included at the top of the file.

  • 55-66: The shouldEject method is clear and concise. It checks if a transaction should be ejected from the CometBFT mempool based on its status. The conditions for ejection are well-documented.

  • 68-81: The txStatus method is well-implemented. It first checks the transaction status in the txpool, and if the transaction is unknown to the pool, it tries to look it up locally. This is a good approach to handle the case when the transaction is unknown to the pool.

cosmos/txpool/ante.go Show resolved Hide resolved
@itsdevbear itsdevbear merged commit 4742e24 into main Oct 31, 2023
13 checks passed
@itsdevbear itsdevbear deleted the pending-b branch October 31, 2023 18:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants