refactor(signing): remove polar and eth_secp256k1
#1191
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1191 +/- ##
==========================================
+ Coverage 50.36% 50.83% +0.46%
==========================================
Files 93 85 -8
Lines 5168 4855 -313
==========================================
- Hits 2603 2468 -135
+ Misses 2373 2212 -161
+ Partials 192 175 -17
|
WalkthroughThe changes primarily focus on refactoring and cleaning up the codebase. Key modifications include removal of unused functions, updating package imports, renaming methods, and adjusting function parameters. The code also introduces new interfaces and their implementations for transaction serialization. Additionally, there are updates to test cases and scripts, reflecting the changes made in the main code. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 11
Files ignored due to filter (20)
- cosmos/crypto/keys/ethsecp256k1/keys.pb.go
- cosmos/go.mod
- cosmos/go.sum
- e2e/hive/clients/polard/config/genesis.json
- e2e/localnet/go.mod
- e2e/localnet/go.sum
- e2e/precompile/polard/config/addrbook.json
- e2e/precompile/polard/config/app.toml
- e2e/precompile/polard/config/client.toml
- e2e/precompile/polard/config/config.toml
- e2e/precompile/polard/config/genesis.json
- e2e/precompile/polard/config/gentx/gentx-4354a78701907c06155caf2bf230ba7988115799.json
- e2e/precompile/polard/config/node_key.json
- e2e/precompile/polard/config/priv_validator_key.json
- e2e/precompile/polard/config/priv_validator_state.json
- e2e/testapp/go.mod
- e2e/testapp/go.sum
- go.work.sum
- tools/go.mod
- tools/go.sum
Files selected for processing (43)
- Makefile (3 hunks)
- contracts/bindings/cosmos/precompile/bank/i_bank_module.abigen.go (2 hunks)
- contracts/bindings/testing/governance/governance_wrapper.abigen.go (1 hunks)
- contracts/src/cosmos/precompile/Bank.sol (1 hunks)
- cosmos/lib/ante/minimal.go (1 hunks)
- cosmos/lib/conversions.go (2 hunks)
- cosmos/lib/signing/signers.go (1 hunks)
- cosmos/lib/tx/serializer.go (1 hunks)
- cosmos/miner/miner.go (3 hunks)
- cosmos/precompile/bank/bank.go (2 hunks)
- cosmos/precompile/bank/bank_test.go (3 hunks)
- cosmos/precompile/governance/governance_test.go (1 hunks)
- cosmos/precompile/governance/testutil.go (1 hunks)
- cosmos/testing/utils/setup.go (2 hunks)
- cosmos/txpool/handler.go (2 hunks)
- cosmos/txpool/handler_test.go (2 hunks)
- cosmos/txpool/mempool.go (1 hunks)
- cosmos/txpool/mempool_test.go (2 hunks)
- cosmos/txpool/mocks/tx_serializer.go (1 hunks)
- cosmos/types/config.go (2 hunks)
- cosmos/types/config_test.go (1 hunks)
- cosmos/x/evm/genesis.go (1 hunks)
- cosmos/x/evm/keeper/msg_server.go (1 hunks)
- cosmos/x/evm/types/tx.go (1 hunks)
- e2e/hive/clients/README.md (1 hunks)
- e2e/hive/clients/polard/hive-init.sh (2 hunks)
- e2e/localnet/network/fixture.go (2 hunks)
- e2e/precompile/contracts/bank/bank_test.go (3 hunks)
- e2e/precompile/contracts/gov/governance_test.go (3 hunks)
- e2e/precompile/contracts/staking/staking_test.go (2 hunks)
- e2e/precompile/polard/start-node.sh (1 hunks)
- e2e/testapp/app.go (5 hunks)
- e2e/testapp/app_config.go (1 hunks)
- e2e/testapp/docker/local/docker-init.sh (2 hunks)
- e2e/testapp/docker/seed/init.sh (1 hunks)
- e2e/testapp/docker/seed/scripts/seed0-init-step1.sh (1 hunks)
- e2e/testapp/docker/seed/scripts/seed0-init-step2.sh (1 hunks)
- e2e/testapp/docker/seed/scripts/seed1-init-step1.sh (1 hunks)
- e2e/testapp/docker/validator/scripts/val0-init-step1.sh (1 hunks)
- e2e/testapp/docker/validator/scripts/val1-init-step1.sh (1 hunks)
- e2e/testapp/entrypoint.sh (3 hunks)
- e2e/testapp/polard/cmd/root.go (3 hunks)
- tools/tools.go (1 hunks)
Files not summarized due to errors (1)
- contracts/bindings/testing/governance/governance_wrapper.abigen.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (25)
- Makefile
- contracts/bindings/cosmos/precompile/bank/i_bank_module.abigen.go
- contracts/src/cosmos/precompile/Bank.sol
- cosmos/lib/conversions.go
- cosmos/lib/signing/signers.go
- cosmos/precompile/bank/bank.go
- cosmos/precompile/bank/bank_test.go
- cosmos/txpool/mempool.go
- cosmos/types/config.go
- cosmos/types/config_test.go
- cosmos/x/evm/genesis.go
- e2e/hive/clients/README.md
- e2e/hive/clients/polard/hive-init.sh
- e2e/localnet/network/fixture.go
- e2e/precompile/contracts/gov/governance_test.go
- e2e/testapp/app_config.go
- e2e/testapp/docker/seed/init.sh
- e2e/testapp/docker/seed/scripts/seed0-init-step1.sh
- e2e/testapp/docker/seed/scripts/seed0-init-step2.sh
- e2e/testapp/docker/seed/scripts/seed1-init-step1.sh
- e2e/testapp/docker/validator/scripts/val0-init-step1.sh
- e2e/testapp/docker/validator/scripts/val1-init-step1.sh
- e2e/testapp/polard/cmd/root.go
- tools/tools.go
- contracts/bindings/testing/governance/governance_wrapper.abigen.go (diff too large)
Additional comments (Suppressed): 37
cosmos/lib/ante/minimal.go (2)
21-22: The package name has been changed from "hd" to "antelib". Ensure that all references to this package throughout the codebase have been updated accordingly.
28-32: The function
NewMinimalHandleris introduced, which returns ansdk.AnteHandler. This function seems to be replacing the previous testing function. Make sure that this change aligns with your project's requirements and that the new function is properly tested.cosmos/x/evm/keeper/msg_server.go (1)
- 37-42: The method
AsTransaction()has been replaced withUnwrap(). Ensure that the new method provides the same functionality as the old one and that it is compatible with the rest of the codebase.cosmos/precompile/governance/testutil.go (1)
- 80-86: The denomination for staking has been changed from "abgt" to "abera". Ensure that this change is reflected throughout the codebase and does not break any functionality. Also, consider handling the error in a more graceful way than just panicking.
- stakingParams.BondDenom = "abgt" + stakingParams.BondDenom = "abera"cosmos/txpool/mocks/tx_serializer.go (2)
26-49: The method
ToSdkTxhas been updated to include a new parametergasLimit. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that thegasLimitis being used correctly within the function.81-104: The method
ToSdkTxByteshas been updated to include a new parametergasLimit. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that thegasLimitis being used correctly within the function.cosmos/precompile/governance/governance_test.go (1)
- 506-513: The
MsgSubmitProposalstruct has been extended with additional fields:InitialDeposit,Proposer,Metadata, andTitle. Ensure that these new fields are correctly populated and used wherever this struct is utilized. Also, verify if these changes have any impact on the existing functionality or tests.cosmos/testing/utils/setup.go (1)
- 89-94: The Bech32 prefixes have been updated from "polar" to "cosmos". Ensure that this change is reflected across the entire codebase and does not break any functionality.
cosmos/lib/tx/serializer.go (3)
33-35: The
TxSerializerinterface has been updated with two new methods:ToSdkTxandToSdkTxBytes. Ensure that all implementations of this interface have been updated accordingly.42-50: The
NewSerializerfunction is introduced to create a new instance ofTxSerializer. It takes atxConfigand awrapFnas parameters. ThewrapFnis a function that wraps the input into an output of typesdk.Msg. This is a good use of dependency injection, which improves testability and maintainability.87-100: The
ToSdkTxBytesmethod first converts the input into a Cosmos SDK transaction using theToSdkTxmethod, then encodes the transaction into bytes using thetxConfig.TxEncoder(). This method provides a convenient way to convert the input directly into byte format suitable for CometBFT.cosmos/txpool/mempool_test.go (2)
43-63: The test case for the "insert" function when the txpool does not error has been updated. The
evmtypes.NewFromTransactionmethod has been replaced withevmtypes.WrapTx. Ensure that this change is intentional and aligns with the new implementation of theInsertmethod in theMempoolstruct.67-73: The test case for the "insert" function when the txpool errors has also been updated to use
evmtypes.WrapTxinstead ofevmtypes.NewFromTransaction. Confirm that this change is consistent with the new implementation of theInsertmethod in theMempoolstruct.e2e/testapp/entrypoint.sh (2)
31-31: The key algorithm has been changed from
eth_secp256k1tosecp256k1. Ensure that this change is compatible with the rest of your system and that all dependencies are updated accordingly.92-92: The Bech32 prefix has been changed from "polar" to "cosmos". Make sure that this change is reflected everywhere in your codebase where addresses are generated or validated.
cosmos/x/evm/types/tx.go (1)
- 34-44: The function
WrapTxis a replacement for the oldNewFromTransaction. It performs the same operation but now returns an error instead of panicking when the transaction cannot be marshalled. This is a good practice as it allows for better error handling.e2e/precompile/contracts/bank/bank_test.go (3)
74-76: The number of denominations has been reduced from 8 to 7. Ensure that this change is intentional and does not affect other parts of the codebase.
93-98: The
evmDenomMetadataobject and its associated checks have been removed. If this metadata is no longer needed, this change is fine. However, if it's still required elsewhere, consider restoring it or handling it in a different way.137-141: The calls to
GetDenomMetadataandGetSendEnabledhave been removed. If these functions are no longer part of theBankModulecontract, this change is acceptable. Otherwise, ensure that their removal doesn't impact the functionality of the contract.e2e/testapp/docker/local/docker-init.sh (2)
32-32: The key algorithm has been changed from
eth_secp256k1tosecp256k1. Ensure that this change is compatible with the rest of your system and doesn't introduce any breaking changes.80-80: The Bech32 prefix has been updated from "polar" to "cosmos". Make sure all instances of the old prefix have been replaced throughout the codebase, and verify that this change doesn't affect any functionality or compatibility.
e2e/precompile/contracts/staking/staking_test.go (2)
- 55-75: The initialization of the
validatorvariable has been changed fromvalidator = tf.ValAddr()to fetching the first bonded validator from thestakingPrecompile.GetBondedValidatorsfunction. Ensure that this change doesn't affect the test cases where thevalidatorvariable is used, especially if the first bonded validator isn't always the same as the one returned bytf.ValAddr().- validator = tf.ValAddr() + validators, _, err := stakingPrecompile.GetBondedValidators(nil, bindings.CosmosPageRequest{}) + Expect(err).ToNot(HaveOccurred()) + Expect(validators).To(HaveLen(1)) + validator = common.BytesToAddress(validators[0].OperatorAddr[:])
- 85-90: The call to
stakingPrecompile.GetBondedValidatorshas been removed from this test case. If the purpose of this test case is to verify the functionality ofGetBondedValidators, then this removal might be an issue. However, if the focus of the test case is only onGetDelegation, then this change should be fine.- validators, _, err := stakingPrecompile.GetBondedValidators(nil, bindings.CosmosPageRequest{}) - Expect(err).ToNot(HaveOccurred()) - Expect(validators[0].OperatorAddr).To(Equal(validator))cosmos/txpool/handler.go (2)
- 51-55: The method signatures in the
TxSerializerinterface have been updated. Ensure that all implementations of this interface throughout the codebase have been updated to match the new signatures.- SerializeToSdkTx(signedTx *coretypes.Transaction) (sdk.Tx, error) - SerializeToBytes(signedTx *coretypes.Transaction) ([]byte, error) + ToSdkTx(signedTx *coretypes.Transaction, gasLimit uint64) (sdk.Tx, error) + ToSdkTxBytes(signedTx *coretypes.Transaction, gasLimit uint64) ([]byte, error)
- 163-169: The
ToSdkTxBytesmethod now requires a second argument for the gas limit. This change is consistent with the updated interface definition. However, ensure that the gas limit passed here (signedEthTx.Gas()) is correctly calculated and does not introduce any potential issues related to transaction execution or cost.- txBytes, err := h.serializer.SerializeToBytes(signedEthTx) + txBytes, err := h.serializer.ToSdkTxBytes(signedEthTx, signedEthTx.Gas())cosmos/miner/miner.go (5)
34-35: The import paths have been updated. Ensure that the new paths are correct and the packages exist at those locations.
41-45: The
EthTxSerializerinterface has been introduced, replacing the previousevmtypes.TxSerializer. This change affects theserializerfield in theMinerstruct and theInitmethod. Make sure all implementations of this interface correctly implement both methods (ToSdkTxandToSdkTxBytes).48-52: The
serializerfield type in theMinerstruct has been changed fromevmtypes.TxSerializertoEthTxSerializer. Ensure that all references to this field throughout the codebase have been updated accordingly.62-64: The
Initmethod now accepts anEthTxSerializerinstead of anevmtypes.TxSerializer. Verify that all calls to this method have been updated to pass anEthTxSerializer.132-135: The call to
m.serializer.SerializeToBytes(&tx)has been replaced withm.serializer.ToSdkTxBytes(&tx, tx.Gas()). Ensure that the additionaltx.Gas()argument is correctly handled in theToSdkTxBytesmethod implementation.cosmos/txpool/handler_test.go (2)
43-57: The
GinkgoT()function is moved inside theBeforeEachblock. This change should not affect the functionality, but it's important to ensure that all tests still pass after this modification.82-103: The method
SerializeToByteshas been replaced withToSdkTxBytes. Ensure that the new method provides the same functionality as the old one. Also, thecoretypes.LegacyTxnow includes aGasfield in addition toNonce. Make sure this additional field is handled correctly in your application.- serializer.On("SerializeToBytes", mock.Anything).Return([]byte{123}, nil).Once() + serializer.On("ToSdkTxBytes", mock.Anything, mock.Anything).Return([]byte{123}, nil).Once() - coretypes.NewTx(&coretypes.LegacyTx{Nonce: 5}) + coretypes.NewTx(&coretypes.LegacyTx{Nonce: 5, Gas: 10})e2e/testapp/app.go (5)
43-48: The import of
"github.com/cosmos/cosmos-sdk/x/auth/ante"has been removed. Ensure that this does not affect any functionality where the ante handler was used.55-68: Several changes in imports are observed. The package
ethcryptocodecis no longer imported, and new packagesantelib,signinglib,libtx, andcoretypesare introduced. Make sure these changes align with the intended functionality and do not introduce any breaking changes.134-142: The function
signinglib.ProvideNoopGetSigners[*evmv1alpha1.WrappedEthereumTransaction]is now provided to the dependency injection system instead ofevmtypes.ProvideEthereumTransactionGetSigners. Verify that this change is intentional and correctly implemented across the codebase.209-214: The AnteHandler setup has been simplified to use
antelib.NewMinimalHandler(). This is a significant change from the previous version which usedevmante.NewAnteHandler. Ensure that this new handler meets all requirements and doesn't break any existing functionality.272-279: The transaction serializer is now created using
libtx.NewSerializer[*coretypes.Transaction](apiSvr.ClientCtx.TxConfig, evmtypes.WrapTx). Previously, it was created usingevmtypes.NewSerializer(apiSvr.ClientCtx.TxConfig). Confirm that this change is correct and doesn't affect the serialization process.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files ignored due to filter (6)
- polaris/cosmos/x/evm/types/tx.pb.go
- proto/buf.gen.gogo.yaml
- proto/buf.gen.pulsar.yaml
- proto/buf.lock
- proto/buf.yaml
- x/evm/types/tx.pb.go
Files selected for processing (3)
- Makefile (5 hunks)
- scripts/proto_generate.sh (2 hunks)
- scripts/proto_generate_pulsar.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- Makefile
Additional comments (Suppressed): 6
scripts/proto_generate.sh (3)
32-33: The directory change from
cosmos/prototoprotomight affect the path where the script looks for.protofiles. Ensure that this change is intentional and that it doesn't break the proto generation process.47-48: The copy command now copies all contents of
pkg.berachain.dev/polaris/*instead ofpkg.berachain.dev/polaris/cosmos/*. This could potentially include unwanted files or directories in the destination. Verify if this broader scope is intended.52-52: The script
proto_generate_pulsar.shhas been moved fromproto/scripts/toscripts/. Confirm that this script exists at the new location and that all invocations of this script have been updated accordingly.scripts/proto_generate_pulsar.sh (3)
34-35: The script is deleting all files with extensions
.pulsar.go,.pb.go,.cosmos_orm.go, and.pb.gw.goin thecosmos/apidirectory. Ensure that this operation does not unintentionally remove any necessary files.38-38: The
buf generate --template buf.gen.pulsar.yamlcommand is being run in theprotodirectory. Verify that thebuf.gen.pulsar.yamlfile exists in this location and contains the correct configuration for your project.40-40: The
apidirectory is being moved tocosmos/. This could potentially break any absolute paths referencing theapidirectory in your codebase. Make sure to update all such references to reflect this change.- mv api cosmos/ + mv cosmos/api cosmos/
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- cosmos/lib/conversions.go (2 hunks)
Additional comments (Suppressed): 2
cosmos/lib/conversions.go (2)
242-248: The new hunk is identical to the old one. No changes have been made in these lines.
252-277: The time fields
SubmitTime,DepositEndTime,VotingStartTime, andVotingEndTimeare now being checked for nil before conversion to Unix timestamp. This is a good practice as it prevents potential runtime panics due to dereferencing a nil pointer.- SubmitTime: uint64(proposal.SubmitTime.Unix()), - DepositEndTime: uint64(proposal.DepositEndTime.Unix()), - VotingStartTime: uint64(proposal.VotingStartTime.Unix()), - VotingEndTime: uint64(proposal.VotingEndTime.Unix()), + if proposal.SubmitTime != nil { + output.SubmitTime = uint64(proposal.SubmitTime.Unix()) + } + + if proposal.DepositEndTime != nil { + output.DepositEndTime = uint64(proposal.DepositEndTime.Unix()) + } + + if proposal.VotingStartTime != nil { + output.VotingStartTime = uint64(proposal.VotingStartTime.Unix()) + } + + if proposal.VotingEndTime != nil { + output.VotingEndTime = uint64(proposal.VotingEndTime.Unix()) + }
itsdevbear
force-pushed
the
signing-changes
branch
from
42fc034 to
84b87d9
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- cosmos/lib/conversions.go (2 hunks)
Additional comments (Suppressed): 4
cosmos/lib/conversions.go (4)
242-248: The assignment of values to the
outputvariable is done correctly. The error handling for the previous operations is also appropriate.252-261: The assignment of values to the
outputvariable continues correctly here. The fields are being populated as expected.263-277: The addition of nil checks before assigning time-related fields is a good practice to avoid potential runtime errors due to null dereferencing. This change improves the robustness of the code.
- SubmitTime: uint64(proposal.SubmitTime.Unix()), - DepositEndTime: uint64(proposal.DepositEndTime.Unix()), - VotingStartTime: uint64(proposal.VotingStartTime.Unix()), - VotingEndTime: uint64(proposal.VotingEndTime.Unix()), + if proposal.SubmitTime != nil { + output.SubmitTime = uint64(proposal.SubmitTime.Unix()) + } + + if proposal.DepositEndTime != nil { + output.DepositEndTime = uint64(proposal.DepositEndTime.Unix()) + } + + if proposal.VotingStartTime != nil { + output.VotingStartTime = uint64(proposal.VotingStartTime.Unix()) + } + + if proposal.VotingEndTime != nil { + output.VotingEndTime = uint64(proposal.VotingEndTime.Unix()) + }
- 279-279: The function now returns the
outputvariable instead of directly returning a struct literal. This change allows for more flexibility in modifying theoutputvariable before it's returned, which is utilized in the new nil checks for time-related fields.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files ignored due to filter (2)
- e2e/hive/clients/polard/config/app.toml
- e2e/hive/clients/polard/config/genesis.json
Files selected for processing (3)
- e2e/hive/clients/polard/Dockerfile (1 hunks)
- e2e/hive/clients/polard/hive-init.sh (3 hunks)
- e2e/localnet/network/node.go (1 hunks)
Files skipped from review due to trivial changes (3)
- e2e/hive/clients/polard/Dockerfile
- e2e/hive/clients/polard/hive-init.sh
- e2e/localnet/network/node.go
Summary by CodeRabbit
golinestarget from Makefile and updated theformattarget.i_bank_module.abigen.go.Bank.sol.NewMinimalHandlerinminimal.go.SdkProposalToGovProposalfunction inconversions.gofor better error handling.ProvideNoopGetSignersinsigners.go.libtxthat provides aTxSerializerinterface and its implementation.Minerstruct to include a field of typeEthTxSerializer.bank.go.bank_test.goandgovernance_test.go.testutil.go.setup.go.handler.goandmempool.go.handler_test.goandmempool_test.go.tx_serializer.go.config.go.genesis.go.EthTransactionfunction inmsg_server.go.tx.go.README.md.hive-init.sh,start-node.sh,docker-init.sh,init.sh,seed0-init-step1.sh,seed0-init-step2.sh,seed1-init-step1.sh,val0-init-step1.sh,val1-init-step1.sh, andentrypoint.sh.app.goandapp_config.go.root.go,proto_generate.sh, andproto_generate_pulsar.sh.tools.go.