feat(cvm): initial eth support (wires until final step transfer shortcut , eth abi message, erc20)#4182
feat(cvm): initial eth support (wires until final step transfer shortcut , eth abi message, erc20)#4182dzmitry-lahoda merged 9 commits intomainfrom
Conversation
# run Composable node
nix run "github:ComposableFi/composable/refs/pull/4182/merge" --allow-import-from-derivation --extra-experimental-features "flakes nix-command" --no-sandbox --accept-flake-config --option sandbox relaxed# run local Picasso DevNet (for CosmWasm development)
nix run "github:ComposableFi/composable/refs/pull/4182/merge#devnet-picasso" --allow-import-from-derivation --extra-experimental-features "flakes nix-command" --no-sandbox --accept-flake-config --option sandbox relaxed # CosmWasm on Substrate CLI tool
nix run "github:ComposableFi/composable/refs/pull/4182/merge#ccw" --allow-import-from-derivation --extra-experimental-features "flakes nix-command" --no-sandbox --accept-flake-config --option sandbox relaxed # run cross chain devnet with Dotsama and Cosmos nodes
nix run "github:ComposableFi/composable/refs/pull/4182/merge#devnet-xc-fresh" --allow-import-from-derivation --extra-experimental-features "flakes nix-command" --no-sandbox --accept-flake-config --option sandbox relaxed
# or same with docker
nix build "github:ComposableFi/composable/refs/pull/4182/merge#devnet-xc-image" --allow-import-from-derivation --extra-experimental-features "flakes nix-command" --no-sandbox --accept-flake-config --option sandbox relaxed \
&& docker load --input result && docker run -it --entrypoint bash devnet-xc:latest -c /bin/devnet-xc-fresh |
dzmitry-lahoda
changed the title
dzmitry-lahoda
changed the title
dzmitry-lahoda
force-pushed
the
dz/b/9
branch
from
710e797 to
371a846
Compare
| pub fn ibc_ics_20_transfer_shortcut( | ||
| deps: Deps, | ||
| msg: &xc_core::gateway::BridgeForwardMsg, | ||
| ) -> Result<IbcIcs20TransferShortcutRoute, ContractError> { |
There was a problem hiding this comment.
This sounds like -> Result<Option<IbcIcs20TransferShortcutRoute>, ContractError> to me and Ok(None) returned if use_shortcut is false or unset I guess.
There was a problem hiding this comment.
option with/vs result opinionated.
my opinion if there is result there is no need option, given rust made results as ergonomic as options, do not see reason to use option.
also soon rust will have keyword generics around result (at least they say), even more less need to use option.
also in case of error like no asset/connection - neither option/result observable as it tries to invoke general program, so it does not improves debugging.
also some languages ditch options, like Roc, as lead to more onsistentencies, Option, is just Result<T,()>
There was a problem hiding this comment.
If the error this function returns doesn’t matter and is never inspected than this should be Option<IbcIcs20TransferShortcutRoute> then. If the function returns an error and the error is ignored, that raises questions. My understanding of this function is that it can return:
- an error which should be propagated,
Noneif shortcut is disabled orSome(shortcut)if shortcut is enabled.
Thus my suggestion is Result<Option<_>, _>. This isn’t opinionated. It better documents what the function returns.
There was a problem hiding this comment.
If the error this function returns doesn’t matter and is never inspected than this should be Option then
than i have instead of writing ? to collapse it to None manually? right? until rust will make any error to be None ?, i recall it was not the case some time ago.
There was a problem hiding this comment.
if the function returns an error and the error is ignored, that raises questions.
why than not raises question ignoring errors inside function if it returns Option? and error is not ignored, it just tells that there is no shortcut for some reason. later I can log it just to see traces from production on routes resolutions. so i better return error and have logs higher level, than make option and prevent higher level to debug.
There was a problem hiding this comment.
an error which should be propagated,
None if shortcut is disabled or
Some(shortcut) if shortcut is enabled.
Thus my suggestion is Result<Option<_>, _>. This isn’t opinionated. It better documents what the function returns.
I understand these words, but thing is i do not see how it helps ergonomics/debugging, it may help some readers, but that is opinionated.
There was a problem hiding this comment.
than i have instead of writing ? to collapse it to None manually? right? until rust will make any error to be None ?, i recall it was not the case some time ago.
I don’t understand what you’re asking. You’re currently ignoring the error. If the function can fail in a way which needs to be propagated further than it should return Result and you can use ? to handle the error. If the function indicates whether shortcut should be used and returns that shortcut if so than there are no errors to consider and you don’t need ?.
why than not raises question ignoring errors inside function if it returns Option?
Because if the function returns an Option than the implication is that it doesn’t fail. That’s the difference for having Result and Option as separate type. Documenting semantics of the values.
There was a problem hiding this comment.
If I would return Option, but one of methods called inside returns error. Would ? make Result in case of error become None? So I do not have to call something .map_error_to_option().
There was a problem hiding this comment.
I would be happy to document failing to solve shortcut as one of enum variants.
Method tries to get shortcut route, it cannot get, it errors. Why? Because either no connection or shortcut disabled, or other reason. For me option is just another case. Specifically direct IBC ICS20 connection not found. But later for sure will have non direct solutions by unrolling prefix path (need to build index for that so). In this case no direct connection means nothing. So would no direction be error or option? I would prefer error so I can log it for tracing.
There was a problem hiding this comment.
If I would return Option, but one of methods called inside returns error. Would ? make Result in case of error become None? So I do not have to call something .map_error_to_option().
If one of the calls return an error, why isn’t it propagated? (And for function returning Option, dealing with Result is just .ok()?).
Method tries to get shortcut route, it cannot get, it errors. Why? Because either no connection or shortcut disabled, or other reason.
But shortcut being disabled is not an error. Unless I misunderstand something, there are three possible options: shortcut, no shortcut or error. If shortcut is enabled and resolved correctly, caller uses the shortcut; if shortcut is disabled, caller uses full path; and if some error while figuring out error happens, caller propagates that error. This is best modelled by Result<Option<_>, _>. With plain Result<_> the caller must now match on the error type to decide whether error needs to be propagated or ‘no shortcut’ path chosen.
|
|
||
| let coin = Coin::new(amount.0, route.local_native_denom.clone()); | ||
| let (msg, event) = if let Ok(transfer_shortcut) = | ||
| ibc_ics_20_transfer_shortcut(deps.as_ref(), &msg) |
There was a problem hiding this comment.
Why are we ignoring error? As commented on the function, it seems to me that this function should return Result<Option<_>, _> and then this would be if let Some(shortcut) = ibc_ics_20_transfer_shortcut(deps.as_ref(), &msg)?.
There was a problem hiding this comment.
will be in next PR to act according shortcut. now no chains configured to use it.
unimplemented!("add tracking lock for funds return usual cosmos message to transfer as defined in {:?}", transfer_shortcut);
Co-authored-by: Michal Nazarewicz <mina86@mina86.com> Signed-off-by: dzmitry-lahoda <dzmitry@lahoda.pro>
|
@mina86 will improve that big I will not use result<option< , see my comment. |
3 participants
Required for merge:
pr-workflow-check / draft-release-checkis ✅ successMakes review faster:
misclabel if it should not be in release notesReviewers@) or used other form of notification of one person who I think can handle best review of this PR