feat(sequencer): support `IbcAction`s and implement ICS20 incoming transfer application logic #579

noot · 2023-11-14T22:56:12Z

Summary

Adds IBC support for ICS20 token transfers

Background

We want IBC support, specifically for token transfers (eg. using TIA as a gas token).

See the cosmos ICS20 spec: https://github.com/cosmos/ibc/blob/fe150abb629de5c1a598e8c7896a7568f2083681/spec/app/ics-020-fungible-token-transfer/README.md

Changes

bump penumbra version and related deps to include IBC refactor for generic AppHandlers + substore support
implement an IbcAsset type which represents an IBC-transferred-token
update Action proto and native enums to include IbcAction variants, IbcAction is a type defined in penumbra_ibc
implement sequencer::accounts::Ics20Transfer which is the app handler for IbcActions, which is called into during IbcAction::check_stateless and IbcAction::execute
Ics20Transfer contains the actual ICS20 transfer logic; namely, handling inbound IBC packets that are transfers. there are two main cases we handle for transfer logic: a token being received on Astria from another chain, or a notification of an outbound transfer failing, in which case we need to refund the escrowed tokens.
in both cases, we determine if Astria is the source of token we need to issue, and handle the packet accordingly (either minting the synthetic, bridged asset if we are not the source, or sending native Astria tokens from the escrow account to the recipient if we are the source)

Testing

can't really test the transfer functionality without heavier integration testing at the moment, due to the fact that it requires merkle proofs of state of the counterparty chain. in discussions with the penumbra team about a "mock tendermint" test harness we could use to test this in a unit-test-like environment.

Breaking Changelist

none

Related Issues

closes #79

## Summary Replaces `buf generate` by `tonic_build` to generate rust code from protobuf. ## Background `buf generate` relies on the third party plugins `protoc-gen-prost` and `protoc-gen-tonic`, which are not yet updated to prost-build 0.12 and tonic-build 0.10 (and hence prost 0.12, tonic 0.10). This patch moves code generation away from the `buf generate` executable to the Rust `tonic-build` crate, because we need to update prost/tonic and because these projects do not guarantee that code generated with older versions work with newer versions. ## Changes - Replace invoking the `buf generate` executable by using `tonic_build` directly - Use `buf build` to create a file descriptor set that can be fed into `tonic_build` (the latter skips its own invocation of `protoc`) - Clean the generated code by removing all files that are not prefixed with `astria.` (tonic also creates client and server code for tendermint abci; I am not sure why, probably due to the descriptor set) ## Testing Not tested directly; this should not affect the actual code generated. Everything should still compile, all tests should pass. ## Related Issues PR requiring a newer version of penumbra and hence prost: #579 PR againast protoc-gen-prost: neoeinstein/protoc-gen-prost#78

…bc substore

noot · 2023-11-22T22:41:50Z

crates/astria-sequencer/src/sequencer.rs

@@ -21,6 +21,9 @@ use crate::{
 state_ext::StateReadExt as _,
 };

+// TODO: use penumbra's `IBC_SUBSTORE_PREFIX` after they merge #3419


if they merge this before this PR is merged (likely), i will update this PR, otherwise i'll do a follow-up.

SuperFluffy

Just quickly went over this to flag obvious issues, but didn't see anything big outside of implementation specific details.

I am going to do a second round to understand the logic.

SuperFluffy · 2023-11-23T11:39:12Z

Cargo.toml

@@ -41,6 +41,11 @@ once_cell = "1.17.1"
 sha2 = "0.10"
 serde = "1"
 serde_json = "1"
+penumbra-component = { git = "https://github.com/penumbra-zone/penumbra.git", rev = "11d3b524da750c16c376759b03e4bbb3474562d9" }


Do you think we can wait until they make a new release?

i can ask them when that'll be

@noot The plan is to do a tagged release monday next week (Dec 4th) along with our new persistent testnet deployment

nice, thanks!

SuperFluffy · 2023-11-23T11:39:42Z

Cargo.toml

-tendermint-config = "0.34"
-tendermint-proto = "0.34"
-tendermint-rpc = "0.34"
+tendermint = "0.34.0"


Why add the .0? Doesn't change anything, just curios.

i just had this here before i merged your update in and didn't change it lol, so no reason in particular

SuperFluffy · 2023-11-23T11:40:57Z

crates/astria-proto/Cargo.toml

@@ -25,6 +27,8 @@ tendermint-proto = { workspace = true }
 tonic = { workspace = true, optional = true }
 tracing = { workspace = true, optional = true }

+anyhow = "1"


Please don't use anyhow here but type the errors.

SuperFluffy · 2023-11-23T11:45:23Z

crates/astria-proto/src/native/sequencer/asset.rs

+ /// # Errors
+ ///
+ /// - if the denomination string is invalid, ie. does not contain any slashes.
+ pub fn from_denomination(denom: &str) -> anyhow::Result<Self> {


Can use an impl FromStr here. Also two notes:

you have the Denom type in a previous PR. It is surprising that this is called from_denomation but takes a string. Better try_from_str or just impl FromStr.

astria-proto tries to use from_* for infallible methods, and try_from_* for falllible

Please don't introduce anyhow here. The rest of the lib tries hard to type the errors

SuperFluffy · 2023-11-23T11:46:36Z

crates/astria-proto/src/native/sequencer/v1alpha1.rs

@@ -8,6 +8,7 @@ use ed25519_consensus::{
 SigningKey,
 VerificationKey,
 };
+use penumbra_ibc::IbcRelay as IbcAction;


Please no alias, just use IbcRelay

SuperFluffy · 2023-11-23T13:19:42Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

+ // check if escrow account has enough balance to refund user
+ let balance = state
+ .get_ibc_channel_balance(source_channel, asset.id())
+ .await?;


add context

SuperFluffy · 2023-11-23T13:21:16Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

+ true,
+ )
+ .await
+ .map_err(|e| {


Prefer if let Err(e) = over map_err. In the future we will have https://doc.rust-lang.org/std/result/enum.Result.html#method.inspect_err

SuperFluffy · 2023-11-23T13:22:21Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

+ .await
+ .map_err(|e| {
+ tracing::error!(
+ "failed to refund tokens during acknowledge_packet_execute: {}",


Don't format, prefer this (assuming that e is likely an anyhow::Error):

let error: &dyn std::error::Error = e.as_ref(); tracing::error!(error, "failed to <your msg>");

SuperFluffy · 2023-11-23T13:22:51Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

+) -> Result<()> {
+ use prost::Message as _;
+
+ let packet_data = FungibleTokenPacketData::decode(data)?;


context for this and the next lines

SuperFluffy · 2023-11-23T13:23:15Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

+ )
+ .context("invalid receiver address")?;
+
+ if is_source(source_port, source_channel, &asset, is_refund) {


add error context for all the errors in here

SuperFluffy · 2023-11-23T13:30:19Z

Cargo.lock

pulling some heavy dependencies, wow

SuperFluffy · 2023-11-23T13:40:37Z

crates/astria-proto/proto/buf.yaml

@@ -1,7 +1,7 @@
 version: v1
 name: buf.build/astria/astria
 deps:
- - buf.build/tendermint/tendermint
+ - buf.build/penumbra-zone/penumbra


This should pin the specific version of the penumbra proto

isn't it pinned in the buf.lock?

Not the same kind of pinning IMO. The lock file encodes code generation at the time buf was run, whereas pinning in the YAML shows author intent.

SuperFluffy

Some comments re making it easier to understand, but looks good. Thank you!

SuperFluffy · 2023-11-28T16:18:02Z

crates/astria-proto/proto/buf.yaml

@@ -1,7 +1,7 @@
 version: v1
 name: buf.build/astria/astria
 deps:
- - buf.build/tendermint/tendermint
+ - buf.build/penumbra-zone/penumbra


Not the same kind of pinning IMO. The lock file encodes code generation at the time buf was run, whereas pinning in the YAML shows author intent.

SuperFluffy · 2023-11-28T16:20:19Z

crates/astria-proto/src/native/sequencer/asset.rs

+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ Self::InvalidDenomination => {
+ write!(f, "denomination must contain at least one slash")


Suggested change

write!(f, "denomination must contain at least one slash")

f.write_str("denomination must contain at least one slash")

SuperFluffy · 2023-11-28T16:25:47Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

@@ -0,0 +1,349 @@
+use std::str::FromStr;


no need to pull this in, use str::parse, see below

SuperFluffy · 2023-11-28T16:26:58Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

+
+ let packet_data = FungibleTokenPacketData::decode(data)
+ .context("failed to decode packet data into FungibleTokenPacketData")?;
+ let asset = IbcAsset::from_str(&packet_data.denom).context("invalid denomination")?;


Once you have impl'ed FromStr you can use str::parse:

Suggested change

let asset = IbcAsset::from_str(&packet_data.denom).context("invalid denomination")?;

let asset = packet_data.denom.parse::<IbcAsset>().context("failed parsing `denom` field packet data as IbcAsset")?;

SuperFluffy · 2023-11-28T16:27:18Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

+ .await
+ .context("failed to get channel balance in refund_tokens_check")?;
+
+ let packet_amount: u128 = packet_data.amount.parse()?;


SuperFluffy · 2023-11-28T16:28:26Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

Can you add a module doc comment on this explaining (and linking) that the purpose of this module is to implement AppHandlerCheck so that Ics20Transfer can be used for penumbra's check_stateless?

Lots of boiler plate to wire it up and it's tough to understand where the logic comes from.

SuperFluffy · 2023-11-28T16:29:05Z

crates/astria-sequencer/src/transaction/mod.rs

+ act.clone()
+ .with_handler::<crate::accounts::ics20_transfer::Ics20Transfer>()
+ .check_stateless(())
+ .await?;


SuperFluffy · 2023-11-28T16:29:22Z

crates/astria-sequencer/src/transaction/mod.rs

+ act.clone()
+ .with_handler::<crate::accounts::ics20_transfer::Ics20Transfer>()
+ .execute(&mut *state)
+ .await?;


SuperFluffy · 2023-11-28T16:32:17Z

crates/astria-sequencer/src/transaction/mod.rs

+ Action::Ibc(act) => {
+ act.clone()
+ .with_handler::<crate::accounts::ics20_transfer::Ics20Transfer>()
+ .check_stateless(())


That's their (penumbra's) check_stateless, right?

Yeah, I see the penumbra action handler above. can we make this more obvious to the reader to disambiguate between our check_stateless and theirs?

let the_action = act.clone().with_handler::<Ics20Transfer>(); penumbra_component::ActionHandler::check_stateless(&the_action);

SuperFluffy · 2023-11-28T16:37:26Z

crates/astria-sequencer/src/accounts/ics20_transfer.rs

+pub(crate) struct Ics20Transfer;
+
+#[async_trait::async_trait]
+impl AppHandlerCheck for Ics20Transfer {


So this is where the magic is: AppHandlerCheck must be impl'ed to make with_handler work.

Can we get that as a module doc comment?

## Summary Counterpart to #579 which supports incoming ICS20 transfers, this PR implements outgoing ICS20 transfers. ## Background required for IBC token transfer support. See https://github.com/cosmos/ibc/blob/fe150abb629de5c1a598e8c7896a7568f2083681/spec/app/ics-020-fungible-token-transfer/README.md `sendFungibleTokens` for spec. ## Changes - add `Ics20Withdrawal` proto and native type which is an action (maybe rename to `Ics20WithdrawalAction`?) - implement check logic (apart from IBC packet check, check the sender has enough balance) - implement execution logic (escrow tokens if native, or burn if not native) ## Testing not really - same issue as #579 ## Related Issues closes #597

noot added 6 commits November 13, 2023 13:27

update deps to start ibc impl

07c77e2

stub AppHandler impl; update proto Action enum to have ibc_action

d93ce2d

update proto native types, update action handler for ibc action

0272d8f

impl AppHandlerCheck for Ics20Transfer

8cde7b6

impl ics20 transfer logic (stub asset stuff for now)

7b43dfd

Merge branch 'main' of github.com:astriaorg/astria into noot/ibc

40f0d02

github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels Nov 14, 2023

noot temporarily deployed to BUF November 14, 2023 22:56 — with GitHub Actions Inactive

noot mentioned this pull request Nov 14, 2023

chore: bump rust version in ci to 1.73.0 #580

Closed

This was referenced Nov 15, 2023

build(proto): replace buf-generate by tonic_build #581

Merged

chore: bump all dependencies (mainly penumbra, celestia, tendermint) #582

Merged

noot changed the title ~~feat(sequencer): suuport IbcActions and implement ICS20 transfer application logic~~ feat(sequencer): support IbcActions and implement ICS20 transfer application logic Nov 16, 2023

merge

a47b351

noot temporarily deployed to BUF November 17, 2023 12:53 — with GitHub Actions Inactive

merge and implement asset logic in ics20transfer

f16d1b7

noot temporarily deployed to BUF November 20, 2023 20:48 — with GitHub Actions Inactive

fmt toml

5c02ee7

noot temporarily deployed to BUF November 20, 2023 20:49 — with GitHub Actions Inactive

update protos

ab70b3b

noot temporarily deployed to BUF November 20, 2023 21:02 — with GitHub Actions Inactive

noot added 2 commits November 21, 2023 15:35

cleanup

c025c63

Merge branch 'main' of github.com:astriaorg/astria into noot/ibc

221f13f

noot mentioned this pull request Nov 22, 2023

sequencer + IBC: implement ICS20Withdrawal action type #597

Closed

bump penumbra dep to include substore commits, update protos + init i…

5edf2dd

…bc substore

noot had a problem deploying to BUF November 22, 2023 16:53 — with GitHub Actions Failure

noot added 3 commits November 22, 2023 12:29

Merge branch 'main' of github.com:astriaorg/astria into noot/ibc

bc6aa3f

use IbcAsset in ics20transfer

051f1bf

fix proto dep

f0aa699

cargo.toml updates

59d6ad6

noot temporarily deployed to BUF November 22, 2023 18:05 — with GitHub Actions Inactive

noot added 2 commits November 22, 2023 13:12

add todo

5cc8d11

update todos

31846fb

noot temporarily deployed to BUF November 22, 2023 22:41 — with GitHub Actions Inactive

noot commented Nov 22, 2023

View reviewed changes

noot marked this pull request as ready for review November 22, 2023 22:42

noot requested review from joroshiba and SuperFluffy as code owners November 22, 2023 22:42

add specific state key for ibc channel balances

9e336e5

noot temporarily deployed to BUF November 22, 2023 23:10 — with GitHub Actions Inactive

SuperFluffy reviewed Nov 23, 2023

View reviewed changes

Cargo.lock Outdated

Copy link

Member

SuperFluffy Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulling some heavy dependencies, wow

noot reacted with confused emoji

SuperFluffy reviewed Nov 23, 2023

View reviewed changes

noot added 2 commits November 26, 2023 17:00

Merge branch 'main' of github.com:astriaorg/astria into noot/ibc

0e5a19c

remove anyhow from proto, address other comments

0cd6324

noot requested a review from SuperFluffy November 27, 2023 18:05

noot temporarily deployed to BUF November 27, 2023 18:05 — with GitHub Actions Inactive

Merge branch 'main' of github.com:astriaorg/astria into noot/ibc

75c44a6

noot temporarily deployed to BUF November 27, 2023 20:49 — with GitHub Actions Inactive

noot changed the title ~~feat(sequencer): support IbcActions and implement ICS20 transfer application logic~~ feat(sequencer): support IbcActions and implement ICS20 incoming transfer application logic Nov 27, 2023

noot mentioned this pull request Nov 27, 2023

feat(sequencer): implement ICS20 withdrawals #609

Merged

update missed map_err

0e01228

noot temporarily deployed to BUF November 27, 2023 21:27 — with GitHub Actions Inactive

SuperFluffy approved these changes Nov 28, 2023

View reviewed changes

address comments

c33618b

noot temporarily deployed to BUF November 28, 2023 17:33 — with GitHub Actions Inactive

noot merged commit b30ce41 into main Nov 28, 2023
26 of 28 checks passed

noot deleted the noot/ibc branch November 28, 2023 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sequencer): support `IbcAction`s and implement ICS20 incoming transfer application logic #579

feat(sequencer): support `IbcAction`s and implement ICS20 incoming transfer application logic #579

noot commented Nov 14, 2023 •

edited by SuperFluffy

Loading

noot Nov 22, 2023

SuperFluffy left a comment

SuperFluffy Nov 23, 2023

noot Nov 26, 2023

erwanor Nov 26, 2023

noot Nov 27, 2023

SuperFluffy Nov 23, 2023

noot Nov 26, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

SuperFluffy Nov 23, 2023

noot Nov 27, 2023

SuperFluffy Nov 28, 2023

SuperFluffy left a comment

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

SuperFluffy Nov 28, 2023

	write!(f, "denomination must contain at least one slash")
	f.write_str("denomination must contain at least one slash")

	let asset = IbcAsset::from_str(&packet_data.denom).context("invalid denomination")?;
	let asset = packet_data.denom.parse::<IbcAsset>().context("failed parsing `denom` field packet data as IbcAsset")?;

feat(sequencer): support IbcActions and implement ICS20 incoming transfer application logic #579

feat(sequencer): support IbcActions and implement ICS20 incoming transfer application logic #579

Conversation

noot commented Nov 14, 2023 • edited by SuperFluffy Loading

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(sequencer): support `IbcAction`s and implement ICS20 incoming transfer application logic #579

feat(sequencer): support `IbcAction`s and implement ICS20 incoming transfer application logic #579

noot commented Nov 14, 2023 •

edited by SuperFluffy

Loading