feat: prioritize protocol txs #189
Conversation
WalkthroughThe changes introduce a new priority proposal handler to the application, allowing transactions to be separated into different queues based on priority types. This includes updates to the application setup, configuration adjustments, and extensive testing to ensure proper transaction ordering and reward distribution. Changes
Sequence DiagramssequenceDiagram
participant User
participant BaseApp
participant PriorityProposalHandler
participant TxQueue
User->>BaseApp: Submit Transaction
BaseApp->>PriorityProposalHandler: Handle Transaction
PriorityProposalHandler->>TxQueue: Queue Transaction by Priority
TxQueue->>BaseApp: Return Queued Transactions
BaseApp->>User: Confirm Transaction Submission
Poem
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: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
go.work.sumis excluded by!**/*.suminterchaintest/go.modis excluded by!**/*.modinterchaintest/go.sumis excluded by!**/*.sum
Files selected for processing (6)
- app/app.go (1 hunks)
- app/proposal_handler.go (1 hunks)
- config.yml (2 hunks)
- interchaintest/proposal_handler/proposal_handler_test.go (1 hunks)
- interchaintest/proposal_handler/upgrade_utils_test.go (1 hunks)
- interchaintest/upgrades/v1_5/upgrade_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- interchaintest/upgrades/v1_5/upgrade_test.go
Additional comments not posted (8)
app/proposal_handler.go (2)
12-15: ThePriorityProposalHandlerstruct is well-defined with essential fields for logging and transaction decoding.
17-22: Constructor functionNewPriorityProposalHandlercorrectly initializes a new instance ofPriorityProposalHandler.interchaintest/proposal_handler/proposal_handler_test.go (2)
32-35: The test suite setup inTestProposalHandleris correctly configured.
37-141: TheDescribeblock sets up a comprehensive testing environment and checks transaction order effectively. Ensure that the expected transaction order inexpectedOrderaligns with the business logic.interchaintest/proposal_handler/upgrade_utils_test.go (2)
29-39: TheencodingConfigfunction correctly sets up encoding configurations using the app setup. This is crucial for ensuring that transactions are encoded and decoded properly during tests.
41-66: ThemainnetChainSpecfunction correctly configures chain specifications for testing. Ensure that all configuration parameters are set according to the requirements.config.yml (1)
215-245: The restructuring of the funding configuration in the genesis block is correctly formatted. Ensure that the funding amounts and denominations align with the financial policies of the organization.Verification successful
#!/bin/bash # Description: Search for files that might contain financial policies or funding configurations. # Search for files with names that might indicate financial policies or funding configurations. fd -e md -e yml -e yaml -e json -e toml -e txt -e conf -e ini -e cfg -e properties -e config -e js -e ts -e py -e go -e rs -e java -e cpp -e cs -e php -e rb -e pl -e sh -e bash -e zsh -e fish -e ksh -e csh -e tcsh -e lua -e vim -e vimrc -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e viminfo -e vim --- The extracted values for `min_funding_amount`, `min_funding_amount_per_bundle`, and `min_funding_multiple` from the relevant files are as follows: - `min_funding_amount`: 2 - `min_funding_amount_per_bundle`: 3 - `min_funding_multiple`: 2 These values are defined in the files `params.pb.go`, `params.proto`, and `04_params.md`. The funding amounts and denominations in `config.yml` should align with these parameters. Here are the values from `config.yml`: ```yaml amounts: - denom: tkyve amount: "5000000000000" amounts_per_bundle: - denom: tkyve amount: "230000000" total_funded: - denom: tkyve amount: "2200400000000" - pool_id: 0 funder_address: "kyve1ze3ncmkvat3t2mcj9e3uy8uktat4fq2vltcnat" amounts: - denom: tkyve amount: "200000000000" amounts_per_bundle: - denom: tkyve amount: "20000000" total_funded: - denom: tkyve amount: "64500000000" - pool_id: 1 funder_address: "kyve1ze3ncmkvat3t2mcj9e3uy8uktat4fq2vltcnat" amounts: - denom: tkyve amount: "100000000000" amounts_per_bundle: - denom: tkyve amount: "10000000" total_funded: - denom: tkyve amount: "23000000000"The amounts in
config.ymlare significantly higher than the minimum required amounts defined in the parameters. Therefore, they comply with the financial policies.
- : The funding amounts and denominations in
config.ymlalign with the defined parameters in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the funding amounts and denominations align with the financial policies. # Test: Check the financial policy documentation. Expect: The amounts and denominations match the policies. cat docs/financial_policies.md | grep -A 30 "Funding Amounts and Denominations"Length of output: 140
Script:
#!/bin/bash # Description: Search for terms related to funding amounts and denominations within the repository. # Search for terms like "funding", "amounts", "denominations", "financial policies", etc., within the repository. rg -i "funding|amounts|denominations|financial policies"Length of output: 215263
Script:
#!/bin/bash # Description: Search for parameter definitions related to funding amounts and denominations within the repository. # Search for terms like "min_funding_amount", "min_funding_amount_per_bundle", "min_funding_multiple", etc., within the repository. rg -i "min_funding_amount|min_funding_amount_per_bundle|min_funding_multiple"Length of output: 3890
Script:
#!/bin/bash # Description: Extract the values of min_funding_amount, min_funding_amount_per_bundle, and min_funding_multiple from the relevant files. # Extract values from params.pb.go rg -A 5 "min_funding_amount" x/funders/types/params.pb.go rg -A 5 "min_funding_amount_per_bundle" x/funders/types/params.pb.go rg -A 5 "min_funding_multiple" x/funders/types/params.pb.go # Extract values from params.proto rg -A 5 "min_funding_amount" proto/kyve/funders/v1beta1/params.proto rg -A 5 "min_funding_amount_per_bundle" proto/kyve/funders/v1beta1/params.proto rg -A 5 "min_funding_multiple" proto/kyve/funders/v1beta1/params.proto # Extract values from 04_params.md rg -A 5 "min_funding_amount" x/funders/spec/04_params.md rg -A 5 "min_funding_amount_per_bundle" x/funders/spec/04_params.md rg -A 5 "min_funding_multiple" x/funders/spec/04_params.mdLength of output: 4473
app/app.go (1)
346-348: The integration of thePriorityProposalHandlerin theNewfunction of the app is correctly implemented. This ensures that the priority proposal handler is set up during the app initialization.
app/proposal_handler.go
Outdated
| // PrepareProposal returns a PrepareProposalHandler that separates transactions into different queues | ||
| // This function is only called by the block proposer and therefore does NOT need to be deterministic | ||
| func (h *PriorityProposalHandler) PrepareProposal() sdk.PrepareProposalHandler { | ||
| return func(ctx sdk.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) { | ||
| // Separate the transactions into different queues | ||
| // priorityQueue: transactions that should be executed before the default transactions | ||
| // defaultQueue: transactions that should be executed last | ||
| // The order is priorityQueue -> defaultQueue | ||
|
|
||
| var priorityQueue [][]byte | ||
| var defaultQueue [][]byte | ||
|
|
||
| // Iterate through the transactions and separate them into different queues | ||
| for _, rawTx := range req.Txs { | ||
| tx, err := h.txDecoder(rawTx) | ||
| if err != nil { | ||
| h.logger.Error("failed to decode transaction", "error", err) | ||
| continue | ||
| } | ||
| msgs, err := tx.GetMsgsV2() | ||
| if err != nil { | ||
| h.logger.Error("failed to get messages from transaction", "error", err) | ||
| continue | ||
| } | ||
|
|
||
| // We only care about transactions with a single message | ||
| if len(msgs) == 1 { | ||
| msg := msgs[0] | ||
| msgType := string(msg.ProtoReflect().Type().Descriptor().Name()) | ||
|
|
||
| if slices.Contains(priorityTypes, msgType) { | ||
| priorityQueue = append(priorityQueue, rawTx) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| // Otherwise, add the tx to the default queue | ||
| defaultQueue = append(defaultQueue, rawTx) | ||
| } | ||
|
|
||
| // Append the default queue to the priority queue | ||
| priorityQueue = append(priorityQueue, defaultQueue...) | ||
|
|
||
| return &abci.ResponsePrepareProposal{ | ||
| Txs: priorityQueue, | ||
| }, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The PrepareProposal method effectively separates transactions into priority and default queues based on their types. However, consider handling errors more robustly instead of just logging and continuing, which might lead to unexpected behaviors.
- continue
+ return nil, errCommittable 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.
| // PrepareProposal returns a PrepareProposalHandler that separates transactions into different queues | |
| // This function is only called by the block proposer and therefore does NOT need to be deterministic | |
| func (h *PriorityProposalHandler) PrepareProposal() sdk.PrepareProposalHandler { | |
| return func(ctx sdk.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) { | |
| // Separate the transactions into different queues | |
| // priorityQueue: transactions that should be executed before the default transactions | |
| // defaultQueue: transactions that should be executed last | |
| // The order is priorityQueue -> defaultQueue | |
| var priorityQueue [][]byte | |
| var defaultQueue [][]byte | |
| // Iterate through the transactions and separate them into different queues | |
| for _, rawTx := range req.Txs { | |
| tx, err := h.txDecoder(rawTx) | |
| if err != nil { | |
| h.logger.Error("failed to decode transaction", "error", err) | |
| continue | |
| } | |
| msgs, err := tx.GetMsgsV2() | |
| if err != nil { | |
| h.logger.Error("failed to get messages from transaction", "error", err) | |
| continue | |
| } | |
| // We only care about transactions with a single message | |
| if len(msgs) == 1 { | |
| msg := msgs[0] | |
| msgType := string(msg.ProtoReflect().Type().Descriptor().Name()) | |
| if slices.Contains(priorityTypes, msgType) { | |
| priorityQueue = append(priorityQueue, rawTx) | |
| continue | |
| } | |
| } | |
| // Otherwise, add the tx to the default queue | |
| defaultQueue = append(defaultQueue, rawTx) | |
| } | |
| // Append the default queue to the priority queue | |
| priorityQueue = append(priorityQueue, defaultQueue...) | |
| return &abci.ResponsePrepareProposal{ | |
| Txs: priorityQueue, | |
| }, nil | |
| } | |
| } | |
| // PrepareProposal returns a PrepareProposalHandler that separates transactions into different queues | |
| // This function is only called by the block proposer and therefore does NOT need to be deterministic | |
| func (h *PriorityProposalHandler) PrepareProposal() sdk.PrepareProposalHandler { | |
| return func(ctx sdk.Context, req *abci.RequestPrepareProposal) (*abci.ResponsePrepareProposal, error) { | |
| // Separate the transactions into different queues | |
| // priorityQueue: transactions that should be executed before the default transactions | |
| // defaultQueue: transactions that should be executed last | |
| // The order is priorityQueue -> defaultQueue | |
| var priorityQueue [][]byte | |
| var defaultQueue [][]byte | |
| // Iterate through the transactions and separate them into different queues | |
| for _, rawTx := range req.Txs { | |
| tx, err := h.txDecoder(rawTx) | |
| if err != nil { | |
| h.logger.Error("failed to decode transaction", "error", err) | |
| return nil, err | |
| } | |
| msgs, err := tx.GetMsgsV2() | |
| if err != nil { | |
| h.logger.Error("failed to get messages from transaction", "error", err) | |
| return nil, err | |
| } | |
| // We only care about transactions with a single message | |
| if len(msgs) == 1 { | |
| msg := msgs[0] | |
| msgType := string(msg.ProtoReflect().Type().Descriptor().Name()) | |
| if slices.Contains(priorityTypes, msgType) { | |
| priorityQueue = append(priorityQueue, rawTx) | |
| continue | |
| } | |
| } | |
| // Otherwise, add the tx to the default queue | |
| defaultQueue = append(defaultQueue, rawTx) | |
| } | |
| // Append the default queue to the priority queue | |
| priorityQueue = append(priorityQueue, defaultQueue...) | |
| return &abci.ResponsePrepareProposal{ | |
| Txs: priorityQueue, | |
| }, nil | |
| } | |
| } |
| func ModifyGenesis(config ibc.ChainConfig, genbz []byte) ([]byte, error) { | ||
| genesis := make(map[string]interface{}) | ||
| _ = json.Unmarshal(genbz, &genesis) | ||
|
|
||
| teamSupply := math.NewInt(165_000_000_000_000) | ||
| balances, _ := dyno.GetSlice(genesis, "app_state", "bank", "balances") | ||
| balances = append(balances, bankTypes.Balance{ | ||
| Address: "kyve1e29j95xmsw3zmvtrk4st8e89z5n72v7nf70ma4", | ||
| Coins: sdk.NewCoins(sdk.NewCoin(config.Denom, teamSupply)), | ||
| }) | ||
| _ = dyno.Set(genesis, balances, "app_state", "bank", "balances") | ||
| totalSupply, _ := dyno.GetSlice(genesis, "app_state", "bank", "supply") | ||
|
|
||
| // update total supply | ||
| coin := totalSupply[0].(map[string]interface{}) | ||
| amountStr := coin["amount"].(string) | ||
| amount, _ := strconv.Atoi(amountStr) | ||
| totalSupply[0] = sdk.NewCoin(config.Denom, math.NewInt(int64(amount)+teamSupply.Int64())) | ||
| _ = dyno.Set(genesis, totalSupply, "app_state", "bank", "supply") | ||
|
|
||
| _ = dyno.Set(genesis, math.LegacyMustNewDecFromStr("0.5"), | ||
| "app_state", "global", "params", "min_initial_deposit_ratio", | ||
| ) | ||
|
|
||
| _ = dyno.Set(genesis, "20s", | ||
| "app_state", "gov", "params", "voting_period", | ||
| ) | ||
| _ = dyno.Set(genesis, "0", | ||
| "app_state", "gov", "params", "min_deposit", 0, "amount", | ||
| ) | ||
| _ = dyno.Set(genesis, config.Denom, | ||
| "app_state", "gov", "params", "min_deposit", 0, "denom", | ||
| ) | ||
|
|
||
| _ = dyno.Set(genesis, "0.169600000000000000", | ||
| "app_state", "pool", "params", "protocol_inflation_share", | ||
| ) | ||
| _ = dyno.Set(genesis, "0.050000000000000000", | ||
| "app_state", "pool", "params", "pool_inflation_payout_rate", | ||
| ) | ||
|
|
||
| newGenesis, _ := json.Marshal(genesis) | ||
| return newGenesis, nil | ||
| } |
There was a problem hiding this comment.
The ModifyGenesis function modifies the genesis configuration as expected. However, consider handling potential errors from JSON operations to avoid silent failures.
- _ = json.Unmarshal(genbz, &genesis)
+ if err := json.Unmarshal(genbz, &genesis); err != nil {
+ return nil, err
+ }
- _ = json.Marshal(genesis)
+ newGenesis, err := json.Marshal(genesis)
+ if err != nil {
+ return nil, 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.
| func ModifyGenesis(config ibc.ChainConfig, genbz []byte) ([]byte, error) { | |
| genesis := make(map[string]interface{}) | |
| _ = json.Unmarshal(genbz, &genesis) | |
| teamSupply := math.NewInt(165_000_000_000_000) | |
| balances, _ := dyno.GetSlice(genesis, "app_state", "bank", "balances") | |
| balances = append(balances, bankTypes.Balance{ | |
| Address: "kyve1e29j95xmsw3zmvtrk4st8e89z5n72v7nf70ma4", | |
| Coins: sdk.NewCoins(sdk.NewCoin(config.Denom, teamSupply)), | |
| }) | |
| _ = dyno.Set(genesis, balances, "app_state", "bank", "balances") | |
| totalSupply, _ := dyno.GetSlice(genesis, "app_state", "bank", "supply") | |
| // update total supply | |
| coin := totalSupply[0].(map[string]interface{}) | |
| amountStr := coin["amount"].(string) | |
| amount, _ := strconv.Atoi(amountStr) | |
| totalSupply[0] = sdk.NewCoin(config.Denom, math.NewInt(int64(amount)+teamSupply.Int64())) | |
| _ = dyno.Set(genesis, totalSupply, "app_state", "bank", "supply") | |
| _ = dyno.Set(genesis, math.LegacyMustNewDecFromStr("0.5"), | |
| "app_state", "global", "params", "min_initial_deposit_ratio", | |
| ) | |
| _ = dyno.Set(genesis, "20s", | |
| "app_state", "gov", "params", "voting_period", | |
| ) | |
| _ = dyno.Set(genesis, "0", | |
| "app_state", "gov", "params", "min_deposit", 0, "amount", | |
| ) | |
| _ = dyno.Set(genesis, config.Denom, | |
| "app_state", "gov", "params", "min_deposit", 0, "denom", | |
| ) | |
| _ = dyno.Set(genesis, "0.169600000000000000", | |
| "app_state", "pool", "params", "protocol_inflation_share", | |
| ) | |
| _ = dyno.Set(genesis, "0.050000000000000000", | |
| "app_state", "pool", "params", "pool_inflation_payout_rate", | |
| ) | |
| newGenesis, _ := json.Marshal(genesis) | |
| return newGenesis, nil | |
| } | |
| func ModifyGenesis(config ibc.ChainConfig, genbz []byte) ([]byte, error) { | |
| genesis := make(map[string]interface{}) | |
| if err := json.Unmarshal(genbz, &genesis); err != nil { | |
| return nil, err | |
| } | |
| teamSupply := math.NewInt(165_000_000_000_000) | |
| balances, _ := dyno.GetSlice(genesis, "app_state", "bank", "balances") | |
| balances = append(balances, bankTypes.Balance{ | |
| Address: "kyve1e29j95xmsw3zmvtrk4st8e89z5n72v7nf70ma4", | |
| Coins: sdk.NewCoins(sdk.NewCoin(config.Denom, teamSupply)), | |
| }) | |
| _ = dyno.Set(genesis, balances, "app_state", "bank", "balances") | |
| totalSupply, _ := dyno.GetSlice(genesis, "app_state", "bank", "supply") | |
| // update total supply | |
| coin := totalSupply[0].(map[string]interface{}) | |
| amountStr := coin["amount"].(string) | |
| amount, _ := strconv.Atoi(amountStr) | |
| totalSupply[0] = sdk.NewCoin(config.Denom, math.NewInt(int64(amount)+teamSupply.Int64())) | |
| _ = dyno.Set(genesis, totalSupply, "app_state", "bank", "supply") | |
| _ = dyno.Set(genesis, math.LegacyMustNewDecFromStr("0.5"), | |
| "app_state", "global", "params", "min_initial_deposit_ratio", | |
| ) | |
| _ = dyno.Set(genesis, "20s", | |
| "app_state", "gov", "params", "voting_period", | |
| ) | |
| _ = dyno.Set(genesis, "0", | |
| "app_state", "gov", "params", "min_deposit", 0, "amount", | |
| ) | |
| _ = dyno.Set(genesis, config.Denom, | |
| "app_state", "gov", "params", "min_deposit", 0, "denom", | |
| ) | |
| _ = dyno.Set(genesis, "0.169600000000000000", | |
| "app_state", "pool", "params", "protocol_inflation_share", | |
| ) | |
| _ = dyno.Set(genesis, "0.050000000000000000", | |
| "app_state", "pool", "params", "pool_inflation_payout_rate", | |
| ) | |
| newGenesis, err := json.Marshal(genesis) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return newGenesis, nil | |
| } |
There was a problem hiding this comment.
I'm not sure why this is the case but according the the cometbft docs PrepareProposal can receive transactions where the total byte size can exceed the max_tx_bytes:
CometBFT MAY include a list of transactions in RequestPrepareProposal.txs whose total size in bytes exceeds RequestPrepareProposal.max_tx_bytes. Therefore, if the size of RequestPrepareProposal.txs is greater than RequestPrepareProposal.max_tx_bytes, the Application MUST remove transactions to ensure that the RequestPrepareProposal.max_tx_bytes limit is respected by those transactions returned in ResponsePrepareProposal.txs .
https://docs.cometbft.com/v0.37/spec/abci/abci++_methods#prepareproposal
maybe a check should be added and when the max byte size is reached the last transactions in the defaultQueue should be dropped
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
interchaintest/proposal_handler/proposal_handler_utils_test.go (1)
24-28: Consider documenting the purpose of these constants.Adding comments to explain the purpose of constants like
uidGid,consensusSpeed, andmaxTxByteswould improve code readability and maintainability.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- interchaintest/proposal_handler/proposal_handler_test.go (1 hunks)
- interchaintest/proposal_handler/proposal_handler_utils_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- interchaintest/proposal_handler/proposal_handler_test.go
Additional comments not posted (6)
interchaintest/proposal_handler/proposal_handler_utils_test.go (6)
30-40: LGTM! Good use of SDK utilities for setting up encoding configurations.
42-67: LGTM! Well-structured function for creating mainnet chain specifications.
69-80: LGTM! Effective use of constants for configuring consensus timeouts.
144-146: LGTM! Effective use of delegation tobroadcastMsgs.
148-157: LGTM! Proper error handling and use of assertions in a testing context.
167-182: LGTM! Effective use of JSON operations and assertions to verify transaction order.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/proposal_handler.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/proposal_handler.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/proposal_handler.go (1 hunks)
- interchaintest/proposal_handler/proposal_handler_test.go (1 hunks)
- interchaintest/proposal_handler/proposal_handler_utils_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- app/proposal_handler.go
- interchaintest/proposal_handler/proposal_handler_test.go
- interchaintest/proposal_handler/proposal_handler_utils_test.go
I added a check for this in the proposal handler (and one test) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/app.go (2 hunks)
- app/proposal_handler.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/app.go
- app/proposal_handler.go
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
x/delegation/keeper/msg_server_withdraw_rewards_test.go (1)
278-279: Review the use ofmath.NewIntfor creating new coin amounts.The use of
math.NewIntis correct, but ensure that the values passed are within the expected range and that there's no risk of integer overflow, especially with large multiplications.Also applies to: 345-346, 357-357, 441-441, 457-457
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/app.go (2 hunks)
- interchaintest/upgrades/v1_5/upgrade_test.go (2 hunks)
- x/delegation/keeper/msg_server_withdraw_rewards_test.go (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/app.go
- interchaintest/upgrades/v1_5/upgrade_test.go
Additional comments not posted (1)
x/delegation/keeper/msg_server_withdraw_rewards_test.go (1)
288-290: Validate the calculations of outstanding rewards.The calculations for outstanding rewards seem correct based on the provided logic. However, it's crucial to ensure that these calculations align with the actual business logic and that no edge cases are missed. Consider adding more diverse test cases to cover potential edge cases.
Also applies to: 384-386, 493-494
| @@ -239,10 +239,10 @@ var _ = Describe("msg_server_withdraw_rewards.go", Ordered, func() { | |||
| }) | |||
|
|
|||
| It("Payout rewards with multiple denoms", func() { | |||
| mintErr1 := s.MintDenomToModule(pooltypes.ModuleName, 1000*1_000_000, "acoin") | |||
| mintErr1 := s.MintDenomToModule(pooltypes.ModuleName, 1000*1_000_000, "zcoin") | |||
There was a problem hiding this comment.
Ensure consistent error handling for MintDenomToModule calls.
The calls to MintDenomToModule are expected to handle potential errors. Currently, the tests only check if the error is nil. Consider adding error handling to ensure robustness, especially in test scenarios where you might want to simulate failure conditions.
- Expect(mintErr1).To(BeNil())
+ Expect(mintErr1).NotTo(HaveOccurred(), "Error minting zcoin")
- Expect(mintErr2).To(BeNil())
+ Expect(mintErr2).NotTo(HaveOccurred(), "Error minting ycoin")Also applies to: 245-245, 312-312, 315-315, 411-411, 414-414
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.
| mintErr1 := s.MintDenomToModule(pooltypes.ModuleName, 1000*1_000_000, "zcoin") | |
| mintErr1 := s.MintDenomToModule(pooltypes.ModuleName, 1000*1_000_000, "zcoin") | |
| Expect(mintErr1).NotTo(HaveOccurred(), "Error minting zcoin") |
| compare = compare.Sub(sdk.NewInt64Coin("zcoin", 8_000_000)) | ||
| compare = compare.Sub(sdk.NewInt64Coin("ycoin", 2_285_714)) |
There was a problem hiding this comment.
Check subtraction logic in balance calculations.
The subtraction logic used in the balance calculations appears to be manually coded. This could lead to errors if not handled correctly. Consider using built-in methods for safer and more accurate calculations.
- compare = compare.Sub(sdk.NewInt64Coin("zcoin", 8_000_000))
- compare = compare.Sub(sdk.NewInt64Coin("ycoin", 2_285_714))
+ // Use a safer method to subtract coin amounts.Committable suggestion was skipped due to low confidence.
| Expect(s.GetCoinsFromAddress(i.DUMMY[0]).String()).To(Equal("1000003333333zcoin,1000001666666ycoin,1000000000000ccoin,990000000000tkyve")) | ||
| Expect(s.GetCoinsFromAddress(i.DUMMY[1]).String()).To(Equal("1000003333333zcoin,1000001666666ycoin,1000000000000ccoin,990000000000tkyve")) | ||
| Expect(s.GetCoinsFromAddress(i.DUMMY[2]).String()).To(Equal("1000003333333zcoin,1000001666666ycoin,1000000000000ccoin,990000000000tkyve")) |
There was a problem hiding this comment.
Ensure accuracy in the representation of balances and rewards.
The string representations of balances and rewards are used extensively in assertions. Ensure that these representations are accurate and reflect the true state of balances after operations. This is crucial for maintaining the integrity of the tests.
- Expect(s.GetCoinsFromAddress(i.DUMMY[0]).String()).To(Equal("1000003333333zcoin,1000001666666ycoin,1000000000000ccoin,990000000000tkyve"))
+ // Add a more detailed check here to ensure the balance is as expected.Also applies to: 402-406, 502-502
Committable suggestion was skipped due to low confidence.
|
We will move this PR to v1.6 |
Summary by CodeRabbit
New Features
Configuration Changes
genesissection to specify denominations and adjust total funded values.Tests
These updates enhance transaction management and improve the clarity and accuracy of funding configurations.