Quality of life features + txid in error cases + Crate updates. #32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkilpatr
requested review from
ChristianBorst and
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This pull request updates dependencies, adds quality-of-life features, and improves error handling by including transaction IDs in error cases. The changes facilitate better transaction tracking and debugging capabilities.
Changes:
- Updated core dependencies (rand 0.9, secp256k1 0.31, rust_decimal 1.40, bytes 1.11) with corresponding API migrations
- Added
get_txhashutility function andTimeoutErrorSignederror variant to preserve transaction IDs in timeout scenarios - Introduced
re_prefixmethod and EthAddress conversion traits for improved address manipulation - Applied various clippy fixes and code quality improvements
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Dependency version updates (rand, secp256k1, rust_decimal, bytes, actix-rt); downgrade of althea_proto |
| src/utils.rs | Added get_txhash function for computing transaction hashes; updated error handling |
| src/private_key.rs | Migrated to secp256k1 0.31 API (from_byte_array, from_digest); updated rand API usage |
| src/mnemonic/mod.rs | Used is_multiple_of method for cleaner modulo checks; updated rand API |
| src/error.rs | Added TimeoutErrorSigned variant and get_txid method for transaction ID retrieval |
| src/coin.rs | Minor whitespace formatting |
| src/client/send.rs | Enhanced timeout handling to preserve txid; removed unnecessary type cast; fixed documentation |
| src/client/gov/mod.rs | Applied clippy fixes for shorthand field initialization |
| src/address.rs | Added re_prefix method and EthAddress conversion trait implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/utils.rs
Outdated
| /// has some known error | ||
| pub fn check_for_sdk_error(input: &TxResponse) -> Result<(), CosmosGrpcError> { | ||
| // check for gas errors | ||
| // check for gas errors, in this case no txid is retured because the tx never made it to the mempool |
There was a problem hiding this comment.
Spelling error: "retured" should be "returned".
Suggested change
| // check for gas errors, in this case no txid is retured because the tx never made it to the mempool | |
| // check for gas errors, in this case no txid is returned because the tx never made it to the mempool |
Member
Author
|
@copilot open a new pull request to apply changes based on the comments in this thread |
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "ethermint")] |
Contributor
There was a problem hiding this comment.
Should we even have this implemented if it's just going to use the default prefix? It seems like a footgun to me.
Member
Author
There was a problem hiding this comment.
it should probably just be a function with a prefix argument. Remembering to re-prefix when using it will be a pain.
ChristianBorst
approved these changes
Jan 20, 2026
These conversions are useful, but should maybe be explicit functions rather than implicit conversions so that we can educate the user as to how it's a bad idea in some situations.
This is easiser to use than the change prefix function in some contexts
By adding a txid to some error cases we can allow the caller to follow the signed transaction even if the intial publishing call fails or times out.
The one thing of note here is that althea_proto was rolled back because everything in the 10 series is currently yanked.
jkilpatr
force-pushed
the
jkilpatr/txid-returns
branch
2 times, most recently
from
1e68149 to
3532f7f
Compare
These types map to each other, but the type checker doesn't know that and it's too much trouble to tell it that in generated proto code.
jkilpatr
force-pushed
the
jkilpatr/txid-returns
branch
from
3532f7f to
a9beb2e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two things worth noting here