cronos
cronos copied to clipboard
crypto-org-chain
Problem: cosmos-sdk 0.46 is not used
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
- [ ] Have you read the CONTRIBUTING.md?
- [ ] Does your PR follow the C4 patch requirements?
- [ ] Have you rebased your work on top of the latest master?
- [ ] Have you checked your code compiles? (
make) - [ ] Have you included tests for any non-trivial functionality?
- [ ] Have you checked your code passes the unit tests? (
make test) - [ ] Have you checked your code formatting is correct? (
go fmt) - [ ] Have you checked your basic code style is fine? (
golangci-lint run) - [ ] If you added any dependencies, have you checked they do not contain any known vulnerabilities? (
go list -json -m all | nancy sleuth) - [ ] If your changes affect the client infrastructure, have you run the integration test?
- [ ] If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
- [ ] If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
- [ ] If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.
Thank you for your code, it's appreciated! :)
Codecov Report
Merging #578 (249444e) into main (1573a35) will decrease coverage by
0.88%. The diff coverage is11.47%.
@@ Coverage Diff @@
## main #578 +/- ##
==========================================
- Coverage 33.82% 32.93% -0.89%
==========================================
Files 33 34 +1
Lines 1558 1603 +45
==========================================
+ Hits 527 528 +1
- Misses 978 1022 +44
Partials 53 53
| Impacted Files | Coverage Δ | |
|---|---|---|
| app/export.go | 0.00% <0.00%> (ø) |
|
| app/state.go | 0.00% <0.00%> (ø) |
|
| app/upgrades.go | 0.00% <0.00%> (ø) |
|
| x/cronos/keeper/grpc_query.go | 0.00% <0.00%> (ø) |
|
| x/cronos/keeper/ibc.go | 75.91% <ø> (ø) |
|
| x/cronos/keeper/keeper.go | 77.24% <ø> (+0.91%) |
:arrow_up: |
| x/cronos/keeper/params.go | 72.72% <ø> (ø) |
|
| x/cronos/proposal_handler.go | 5.88% <ø> (ø) |
|
| x/cronos/types/codec.go | 0.00% <ø> (ø) |
|
| x/cronos/types/events.go | 0.00% <ø> (ø) |
|
| ... and 4 more |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
The v1 mempool currently has a limitation we might need to consider: priority mempool only allows 1 tx per sender because Tendermint doesn't understand nonces and how to provide local ordering. The SDK needs to change the nonce checks logic.
The v1 mempool currently has a limitation we might need to consider: priority mempool only allows 1 tx per sender because Tendermint doesn't understand nonces and how to provide local ordering. The SDK needs to change the nonce checks logic.
Do you mean the priority order maybe different from nonce order?
The v1 mempool currently has a limitation we might need to consider: priority mempool only allows 1 tx per sender because Tendermint doesn't understand nonces and how to provide local ordering. The SDK needs to change the nonce checks logic.
Do you mean the priority order maybe different from nonce order?
No, the pool will reject the later tx that has a bigger nonce if it has the same sender.
https://github.com/tendermint/tendermint/blob/main/mempool/v1/mempool.go#L484-L502
The v1 mempool currently has a limitation we might need to consider: priority mempool only allows 1 tx per sender because Tendermint doesn't understand nonces and how to provide local ordering. The SDK needs to change the nonce checks logic.
Do you mean the priority order maybe different from nonce order?
No, the pool will reject the later tx that has a bigger nonce if it has the same sender.
https://github.com/tendermint/tendermint/blob/main/mempool/v1/mempool.go#L484-L502
Hmm, I think it'll be reject by checkTx anyway, due to nonce mismatch. But is there a plan to support future list though, to hold the txs with future nonce for a while.
hold txs in mempool after we propose the block could be done by ABCI++ via PrepareProposal:
If the Application considers that tx should not be proposed in this block, e.g., there are other transactions with higher priority, then it should not include it in tx_records. In this case, Tendermint will not remove tx from the mempool.
https://github.com/tendermint/tendermint/blob/v0.36.x/spec/abci%2B%2B/abci++_methods.md#prepareproposal
hold txs in mempool after we propose the block could be done by ABCI++ via
PrepareProposal:If the Application considers that tx should not be proposed in this block, e.g., there are other transactions with higher priority, then it should not include it in tx_records. In this case, Tendermint will not remove tx from the mempool.
https://github.com/tendermint/tendermint/blob/v0.36.x/spec/abci%2B%2B/abci++_methods.md#prepareproposal
that's for the tx that's already in the mempool, but current issue is it won't include multiple txs for same sender in the first place. Even if it include multiple txs in mempoo,, the other issue is it won't include them into blocks in the nonce order.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#modules
The x/param module has been depreacted in favour of each module housing and providing way to modify their parameters. Each module that has parameters that are changable during runtime have an authority, the authority can be a module or user account. The Cosmos-SDK team recommends migrating modules away from using the param module. An example of how this could look like can be found here. The Param module will be maintained until April 18, 2022. At this point the module will reach end of life and be removed from the Cosmos SDK.
@yihuang @thomas-nguy separate PRs for x/cronos and x/gravitybridge to fix it after 0.46 PRs are merged?
https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#modules
The x/param module has been depreacted in favour of each module housing and providing way to modify their parameters. Each module that has parameters that are changable during runtime have an authority, the authority can be a module or user account. The Cosmos-SDK team recommends migrating modules away from using the param module. An example of how this could look like can be found here. The Param module will be maintained until April 18, 2022. At this point the module will reach end of life and be removed from the Cosmos SDK.
@yihuang @thomas-nguy separate PRs for x/cronos and x/gravitybridge to fix it after 0.46 PRs are merged?
https://github.com/crypto-org-chain/cronos/issues/632
as discussed on Slack, this may also need a workaround for the prioritised mempool to allow stable IBC message relaying, but I guess this can be merged first and the mempool workaround can be done in a separate PR?
https://github.com/crypto-org-chain/cronos/issues/634
as discussed on Slack, this may also need a workaround for the prioritised mempool to allow stable IBC message relaying, but I guess this can be merged first and the mempool workaround can be done in a separate PR?
https://github.com/crypto-org-chain/cronos/issues/634