Signer parsing from context #1393
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1393 +/- ##
==========================================
- Coverage 67.09% 67.06% -0.04%
==========================================
Files 226 226
Lines 22425 22483 +58
==========================================
+ Hits 15047 15079 +32
- Misses 7378 7404 +26 ☔ View full report in Codecov by Sentry. |
| /// Attempt to convert a [`Signer`] to a native chain sender account. | ||
| fn sender_account_from_signer(&self, signer: &Signer) -> Option<Self::AccountId>; | ||
|
|
||
| /// Attempt to convert a [`Signer`] to a native chain receiver account. | ||
| fn receiver_account_from_signer(&self, signer: &Signer) -> Option<Self::AccountId>; |
There was a problem hiding this comment.
Can you update the return type to Result<Self::AccountId, HostError>? It’d keep it consistent with our other APIs. Since the account ID isn’t always derivable from the signer, and ibc-rs typically propagates errors originating from the host boundary as HostError, it makes more sense to classify it this way. This also gives hosts the flexibility to include contextual information in the error, which ibc-rs can then pass along.
(This PR seems like a good move from this perspective too.)
| type AccountId; | ||
|
|
||
| /// Attempt to convert a [`Signer`] to a native chain sender account. | ||
| fn sender_account_from_signer(&self, signer: &Signer) -> Option<Self::AccountId>; |
There was a problem hiding this comment.
I suggest renaming these methods to sender_account and receiver_account. The Signer type is currently used for account-like fields everywhere in ibc-rs, but it actually need improvement later since it can be confusing. As you can see, the sender and receiver aren’t actually signers here. So, I'm suggesting to avoid breaking the API naming in the future. Even the args can be updated to sender and receiver.
| .map_err(|_| TokenTransferError::FailedToParseAccount)?; | ||
| let sender = token_ctx_a | ||
| .sender_account_from_signer(&msg.packet_data.sender) | ||
| .ok_or(TokenTransferError::FailedToParseAccount)?; |
There was a problem hiding this comment.
With the update requested in the other comment, this FailedToParseAccount variant can completely be removed.
| @@ -0,0 +1,3 @@ | |||
| - Replace the `TryFrom<Signer>` bound on `AccountId` with new context | |||
There was a problem hiding this comment.
| - Replace the `TryFrom<Signer>` bound on `AccountId` with new context | |
| - [ibc-apps] Replace the `TryFrom<Signer>` bound on `AccountId` with new context |
| @@ -0,0 +1,3 @@ | |||
| - Replace the `TryFrom<Signer>` bound on `AccountId` with new context | |||
There was a problem hiding this comment.
Let's put this under the /unreleased/breaking-changes/ directory.
Based on #1392
Diff: https://github.com/heliaxdev/cosmos-ibc-rs/compare/heliaxdev:cosmos-ibc-rs:tiago/optional-ack-rebased..heliaxdev:cosmos-ibc-rs:tiago/signer-parsing-from-ctx
Description
This PR replaces the
TryFrom<Signer>bound onAccountIdwith new context methods, with the aim of contextually parsingSignerinstances.(Some context: #1392 (comment))
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.