feat(sequencer): implement ICS20 withdrawals #609

noot · 2023-11-27T21:25:17Z

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

…bc substore

…thdrawal

…0-withdrawal

SuperFluffy · 2023-12-07T18:11:13Z

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

+ }
+}
+
+impl From<raw::IbcHeight> for IbcHeight {


We have a Protobuf trait for conversions on foreign types. Can you impl it instead of From? Use type Error = ::std::convert::Infallible

I implemented From because the conversion never fails, so I left this for now, but I can change to a from_raw or something instead?

Your logic for implementing From is sound. The reason I don't like From here is consistency. The rest of the crate is has from_raw and try_from_raw so readers will expect to find it.

I intend to expand the Protobuf trait to all our proto types to document all (raw, native) pairs.

SuperFluffy · 2023-12-07T18:16:10Z

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

+ }
+}
+
+impl Error for Ics20WithdrawalError {}


This impl would swallow all sources. This should implement the fn source(&self) -> Option<&dyn std::error::Error> that returns the sources for those variants that are actually wrapping errors.

You can avoid some boiler plate by using thiserror which I will probably use to migrate all errors over.

I would also like to make use of an extra indirection like below to hide the actual errors behind an opaque facade (see how the top level is pub but the variants are priv):

pub struct Ics20WithdrawalError { kind: Ics20WithdrawalErrorKind, } enum Ics20WithdrawalErrorKind { TheOtherVariants, }

Note on this: in #646 I have reimplemented all errors in terms of thiserror using something like this:

#[derive(thiserror::Error)] #[error(transparent)] struct PublicError(PrivateError); #[derive(thiserror::Error)] enum PrivateError{}

SuperFluffy · 2023-12-07T18:20:09Z

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

@@ -563,6 +580,7 @@ enum ActionErrorKind {
 SudoAddressChange(SudoAddressChangeActionError),
 Mint(MintActionError),
 Ibc(Box<dyn Error + Send + Sync>),
+ Ics20Withdrawal(Box<dyn Error + Send + Sync>),


I think this is not necessary, you can toss the actual error type in here. We used the boxed error trait object because I don't want anyhow in here. :)

SuperFluffy · 2023-12-07T18:21:49Z

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

+ pub source_channel: ChannelId,
+}
+
+impl From<Ics20Withdrawal> for FungibleTokenPacketData {


Can you impl this as an inherent method on Ics20Withdrawal? fn to_fungible_token_packet_data(&self)

SuperFluffy · 2023-12-07T18:23:00Z

crates/astria-proto/Cargo.toml

 penumbra-ibc = { workspace = true }
 penumbra-proto = { workspace = true }
 prost = { workspace = true }
 prost-types = { workspace = true }
 serde = { workspace = true, optional = true }
+serde_json = { workspace = true }


Note below: please remove

SuperFluffy · 2023-12-07T18:43:02Z

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

+ .await
+ .context("failed getting `from` account balance for transfer")?;
+ ensure!(
+ from_transfer_balance > self.amount,


> or >=?

SuperFluffy · 2023-12-07T18:44:04Z

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

+ );
+
+ state
+ .put_account_balance(from, self.denom.id(), from_transfer_balance - self.amount)


Please use checked math here

the "insufficient funds for transfer" check above covers this

SuperFluffy · 2023-12-07T18:44:26Z

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

+ .put_ibc_channel_balance(
+ &self.source_channel,
+ self.denom.id(),
+ channel_balance + self.amount,


checked math

SuperFluffy · 2023-12-07T18:45:38Z

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

+ .context("failed to get channel balance")?;
+
+ state
+ .put_ibc_channel_balance(


Do we need to worry about making the entirety of these steps a transaction similar to how SQL transactions are atomic (if one fails, all fail or are rolled back)?

mentioned this before, but check deliver_tx, it starts an atomic tx for the db that's only committed if the tx succeeds

OK, cool. I have to go over how new entries in the DB are created to understand the flow again.

SuperFluffy · 2023-12-07T18:47:27Z

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

+};
+
+#[derive(Debug, Clone)]
+pub struct Ics20Withdrawal {


I increasingly lean toward abstaining from making all of these ~~private~~ pub by default.

Can you lock it down?

you mean make the fields private? yeah i changed it, made getters instead

Yeah, that's what I meant! Sorry, I meant to write public instead of private.

…thdrawal

SuperFluffy

Looks good know.

I have added a few comments that could be addressed before merge.

Can we rebase this on top of the refactors in #644 #646 in before we merge this?

SuperFluffy · 2023-12-13T09:59:13Z

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

+ }
+}
+
+impl From<IbcHeight> for raw::IbcHeight {


Remove so there is only a single way to do this.

SuperFluffy · 2023-12-13T10:01:50Z

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

+ .put_account_balance(
+ from,
+ self.denom().id(),
+ from_transfer_balance - self.amount(),


You can do checked math && the check in one go:

let Some(new_balance) = from_transfer_balance.checked_sub(self.amount()) else { bail!("insufficient funds for transfer"); }

This addresses my ask for checked math while also defending against accidental refactor.

SuperFluffy · 2023-12-13T10:05:52Z

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

+ .context("failed to get channel balance")?;
+
+ state
+ .put_ibc_channel_balance(


OK, cool. I have to go over how new entries in the DB are created to understand the flow again.

SuperFluffy · 2023-12-13T10:21:39Z

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

+ amount: Some(self.amount.into()),
+ denom: self.denom.to_string(),
+ destination_chain_address: self.destination_chain_address,
+ return_address: self.return_address.0.to_vec(),


Suggested change

return_address: self.return_address.0.to_vec(),

return_address: self.return_address.to_vec(),

Address has a to_vec method

SuperFluffy · 2023-12-13T10:27:30Z

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

+ }
+}
+
+impl crate::native::Protobuf for IbcHeight {


Since all the fields of raw::IbcHeight are Copy you don't have to impl all the trait methods. The trait definition provides default implementations for try_from_raw (in terms of try_from_raw_ref) and into_raw (in terms of to_raw). So if you impl to_raw and try_from_raw_ref you get the other two for free.

try_from_raw and into_raw can be implemented by non-copy types to avoid allocations.

SuperFluffy · 2023-12-13T10:29:18Z

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

+
+#[allow(clippy::module_name_repetitions)]
+#[derive(Debug)]
+pub enum Ics20WithdrawalError {


See how this is done in PR #646: derive thiserror::Error for this and split into a public wrapper type that has #[error(transparent)] and a private error type that has all the enums.

SuperFluffy · 2023-12-13T10:29:47Z

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

@@ -0,0 +1,140 @@
+use anyhow::{
+ ensure,
+ Context,


Suggested change

Context,

Context as _,

SuperFluffy · 2023-12-13T10:32:26Z

proto/astria/sequencer/v1alpha1/transaction.proto

@@ -75,3 +76,18 @@ message MintAction {
 bytes to = 1;
 astria.primitive.v1.Uint128 amount = 2;
 }
+
+message Ics20Withdrawal {


Can you add comments to this type, what IBC spec it matches, maybe with links. and how this is used?

Please also cover the fields and their constraints, i.e. the expected format of the strings, the number of bytes for things like the return address, units of the time.

yep, added more description, and added a longer comment + spec link to the rust type

SuperFluffy · 2023-12-13T10:32:40Z

proto/astria/sequencer/v1alpha1/transaction.proto

+ string destination_chain_address = 3;
+ bytes return_address = 4;
+ IbcHeight timeout_height = 5;
+ uint64 timeout_time = 6;


units? greater 0? Also is this used as a time ("timeout at time T") or as a duration ("timeout in duration D seconds")? This here is probably used a duration, so I would name it thus.

I have seen in other projects that that lead to confusion, so IMO it's worth bikeshedding this naming.

copied the comments on the rust type here - this is a unix timestamp

SuperFluffy · 2023-12-13T10:32:51Z

proto/astria/sequencer/v1alpha1/transaction.proto

+ astria.primitive.v1.Uint128 amount = 1;
+ string denom = 2;
+ string destination_chain_address = 3;
+ bytes return_address = 4;


Our address type? 20 bytes?

yeah it is, in the rust Ics20Withdrawal type this is an Address

noot added 30 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

merge

a47b351

merge and implement asset logic in ics20transfer

f16d1b7

fmt toml

5c02ee7

update protos

ab70b3b

cleanup

c025c63

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

221f13f

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

5edf2dd

…bc substore

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

add todo

5cc8d11

update todos

31846fb

add specific state key for ibc channel balances

9e336e5

add ics20_withdrawal to action proto

96a9156

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

0e5a19c

remove anyhow from proto, address other comments

0cd6324

Merge branch 'main' of github.com:astriaorg/astria into noot/ics20-wi…

71aba3e

…thdrawal

wip

8e45e04

merge

1d49d36

ics20withdrawal protos types added

5a8a396

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

75c44a6

implement Ics20Withdrawal checks and execution

346ce35

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

a363a2d

…0-withdrawal

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

update penumbra commit

7bcd312

Base automatically changed from noot/ibc to main November 28, 2023 17:40

merge w main

1833537

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

merge

4990fde

noot temporarily deployed to BUF December 1, 2023 18:11 — with GitHub Actions Inactive

noot added 2 commits December 1, 2023 14:32

use penumbra_ibc::IBC_SUBSTORE_PREFIX

21d2f37

cleanup

7348b72

noot temporarily deployed to BUF December 1, 2023 19:33 — with GitHub Actions Inactive

update protos

289336b

noot temporarily deployed to BUF December 1, 2023 20:33 — with GitHub Actions Inactive

fix protos

cc9d2ed

noot temporarily deployed to BUF December 1, 2023 20:40 — with GitHub Actions Inactive

SuperFluffy reviewed Dec 7, 2023

View reviewed changes

merge w main

96cf34e

noot temporarily deployed to BUF December 9, 2023 06:19 — with GitHub Actions Inactive

noot added 2 commits December 12, 2023 16:34

Merge branch 'main' of github.com:astriaorg/astria into noot/ics20-wi…

37070ee

…thdrawal

address comments

2a303ae

noot requested a review from SuperFluffy December 13, 2023 03:38

noot temporarily deployed to BUF December 13, 2023 03:38 — with GitHub Actions Inactive

SuperFluffy approved these changes Dec 13, 2023

View reviewed changes

address comments

4aedb4c

noot temporarily deployed to BUF December 13, 2023 19:20 — with GitHub Actions Inactive

update timeout_time comment

fcd538f

noot temporarily deployed to BUF December 13, 2023 19:39 — with GitHub Actions Inactive

update proto

be760e7

noot temporarily deployed to BUF December 13, 2023 19:43 — with GitHub Actions Inactive

noot merged commit c353cf2 into main Dec 13, 2023
30 checks passed

noot deleted the noot/ics20-withdrawal branch December 13, 2023 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sequencer): implement ICS20 withdrawals #609

feat(sequencer): implement ICS20 withdrawals #609

noot commented Nov 27, 2023 •

edited

Loading

SuperFluffy Dec 7, 2023

noot Dec 13, 2023

SuperFluffy Dec 13, 2023 •

edited

Loading

SuperFluffy Dec 7, 2023

SuperFluffy Dec 13, 2023

SuperFluffy Dec 7, 2023 •

edited

Loading

SuperFluffy Dec 7, 2023

SuperFluffy Dec 7, 2023

SuperFluffy Dec 7, 2023

SuperFluffy Dec 7, 2023

noot Dec 13, 2023

SuperFluffy Dec 7, 2023

SuperFluffy Dec 7, 2023

noot Dec 13, 2023

SuperFluffy Dec 13, 2023

SuperFluffy Dec 7, 2023 •

edited

Loading

noot Dec 13, 2023

SuperFluffy Dec 13, 2023

SuperFluffy left a comment

SuperFluffy Dec 13, 2023

SuperFluffy Dec 13, 2023

SuperFluffy Dec 13, 2023

SuperFluffy Dec 13, 2023

SuperFluffy Dec 13, 2023

SuperFluffy Dec 13, 2023

SuperFluffy Dec 13, 2023

SuperFluffy Dec 13, 2023 •

edited

Loading

noot Dec 13, 2023

SuperFluffy Dec 13, 2023 •

edited

Loading

noot Dec 13, 2023

SuperFluffy Dec 13, 2023

noot Dec 13, 2023

	return_address: self.return_address.0.to_vec(),
	return_address: self.return_address.to_vec(),

feat(sequencer): implement ICS20 withdrawals #609

feat(sequencer): implement ICS20 withdrawals #609

Conversation

noot commented Nov 27, 2023 • edited Loading

Summary

Background

Changes

Testing

Related Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy Dec 7, 2023 • edited Loading

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 Dec 7, 2023 • edited Loading

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

SuperFluffy Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy Dec 13, 2023 • edited Loading

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

noot commented Nov 27, 2023 •

edited

Loading

SuperFluffy Dec 13, 2023 •

edited

Loading

SuperFluffy Dec 7, 2023 •

edited

Loading

SuperFluffy Dec 7, 2023 •

edited

Loading

SuperFluffy Dec 13, 2023 •

edited

Loading

SuperFluffy Dec 13, 2023 •

edited

Loading