feat(sequencer): implement support for arbitrary assets #568

noot · 2023-11-08T20:41:53Z

Summary

This PR implements support for arbitrary assets for fee payments and balance/transfers inside the sequencer. Currently, only one asset exists (the native asset, RIA), but after IBC is implemented we can potentially have many arbitrary assets on the sequencer chain.

Background

We need to support multiple assets for IBC, and also support fee payments in different token types (probably ibc-TIA)

Changes

implement a Denom type, which represents the base (smallest) denomination of an arbitrary asset
implement an Id type, which is a hash of the denomination "trace" (see https://github.com/cosmos/ibc-go/blob/main/docs/architecture/adr-001-coin-source-tracing.md for details) in this PR, since there is no IBC, the Id is just the hash of the base denomination.
update UnsignedTransaction to have an optional fee_asset field; if this is unset, the native asset is used for fee payment
update Transfer to have an optional asset field; if this is unset, the native asset is the asset transferred
update the sequencer app to have a NATIVE_ASSET global
update the genesis state to have a native_asset_base_denomination field. the native asset should be set in the genesis file, and stored in (non-consensus) storage for retrieval upon startups. then the NATIVE_ASSET global is set upon startup by reading from storage.

Testing

unit tests, ran it locally

Breaking Changelist

UnsignedTransaction and Transfer now have an extra (optional) field.

Related Issues

closes #559

…r-asset

…e asset

SuperFluffy · 2023-11-20T12:46:13Z

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

+    }
+
+    #[must_use]
+    pub fn id(&self) -> Id {


Can you add little doc strings to this that also explain what these things are (i.e. hash of the base denom, what it's used for).

SuperFluffy · 2023-11-20T12:47:52Z

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

+
+/// Asset ID, which is the hash of the denomination trace.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct Id([u8; 32]);


Is there an advantage of having this be a self-standing type as opposed to leaving this as an implementation detail fo the Denom type?

One need not match every proto type with a "native" type.

yeah, there will be an IbcAsset type also which will use this, so I'd like to keep it named

SuperFluffy · 2023-11-20T12:49:05Z

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

    pub nonce: u32,
    pub actions: Vec<Action>,
+    /// asset to use for fee payment.
+    pub fee_asset_id: asset::Id,


See above, not opposed to types, just wondering if leaving this as a bare [u8; 32] would be fine.

SuperFluffy · 2023-11-20T12:50:54Z

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

+        // to cover both the fee and the amount transferred
+        if fee_asset_id == &transfer_asset_id {
+            ensure!(
+                from_fee_balance >= self.amount + TRANSFER_FEE,


Can we use let Some(from_fee_balance) = self.amount.checked_add(TRANSFER_FEE) else {}; here? These things are usually fine, but I'd like us to be extra cautious here because the worst case scenario (however unlikely) is bad.

SuperFluffy · 2023-11-20T12:52:13Z

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

+                "insufficient funds for fee payment"
+            );
+
+            let from_transfer_balance = state.get_account_balance(from, &transfer_asset_id).await?;


Suggested change

let from_transfer_balance = state.get_account_balance(from, &transfer_asset_id).await?;

let from_transfer_balance = state.get_account_balance(from, &transfer_asset_id)

.await

.context("failed getting account balance while checking that enough funds are available")?;

SuperFluffy · 2023-11-20T12:53:09Z

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

+                .put_account_balance(
+                    from,
+                    &transfer_asset_id,
+                    from_balance - (self.amount + TRANSFER_FEE),


As above, can we use checked math for all of this? I.e. checked_sub + checked_add.

Can put that into a utility function to make the purpose clear.

SuperFluffy · 2023-11-20T12:56:15Z

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

        &self,
        _state: &S,
        _from: Address,
+        _fee_asset_id: &asset::Id,


Isn't Id: Copy? Could just _fee_asset_id: asset::Id

SuperFluffy · 2023-11-20T12:57:25Z

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

+                )
+                .context("failed updating `from` account balance")?;
+            state
+                .put_account_balance(self.to, &transfer_asset_id, to_balance + self.amount)


checked math?

SuperFluffy · 2023-11-20T12:57:36Z

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

+            // otherwise, just transfer the transfer asset and deduct fee from fee asset balance
+            // later
+            state
+                .put_account_balance(from, &transfer_asset_id, from_balance - self.amount)


checked math?

SuperFluffy · 2023-11-20T12:57:42Z

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

+                .put_account_balance(from, &transfer_asset_id, from_balance - self.amount)
+                .context("failed updating `from` account balance")?;
+            state
+                .put_account_balance(self.to, &transfer_asset_id, to_balance + self.amount)


checked math?

SuperFluffy · 2023-11-20T12:57:48Z

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

+                .await
+                .context("failed getting `from` account balance for fee payment")?;
+            state
+                .put_account_balance(from, fee_asset_id, from_fee_balance - TRANSFER_FEE)


checked math?

SuperFluffy · 2023-11-20T12:58:36Z

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

+            state
+                .put_account_balance(self.to, &transfer_asset_id, to_balance + self.amount)
+                .context("failed updating `to` account balance")?;
+        } else {


If any one of the steps in this else block fails, don't we need some kind of rollback mechanism to stay consistent?

this is in a StateDelta, so the state changes will not be committed if this fails. see this part of deliver_tx:

let mut state_tx = self .state .try_begin_transaction() .expect("state Arc should be present and unique"); transaction::execute(&signed_tx, &mut state_tx) .await .context("failed executing transaction")?; state_tx.apply();

if the tx fails to execute, the changes aren't applied to the StateDelta.

SuperFluffy · 2023-11-20T13:00:13Z

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

+    }
+
+    #[must_use]
+    pub fn as_bytes(&self) -> &[u8; 32] {


You could add AsRef for this.

SuperFluffy · 2023-11-20T13:02:53Z

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

+    format!(
+        "{}/balance/{}",
+        storage_key(&address.encode_hex::<String>()),
+        asset.as_bytes().encode_hex::<String>()


See my comment above: hex::ToHex is implemented for all T: AsRef<[u8]>, so you could just impl AsRef<[u8]> for Id to make this a bit more elegant

SuperFluffy · 2023-11-20T13:07:12Z

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

@@ -0,0 +1,21 @@
+use std::sync::OnceLock;


Cool. This makes sequencer need msrv 1.70, but it's taken care of in #582

SuperFluffy

Looks good. The fact that the native asset is encoded now I like.

Other than some requests for error context, please also have a look at this comment (can't link it; it gives you the file, which is confusing, quoting; need grep to find it):

If any one of the steps in this else block fails, don't we need some kind of rollback mechanism to stay consistent?

Also, I would very much like to go overboard with checked math for the fee calculations as a safety precaution.

…r-asset

noot added 6 commits November 6, 2023 19:29

create asset::Denom type

4eb22f9

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

00ed3a8

…r-asset

update Transfer to contain asset ID, update balance storage to includ…

baeca62

…e asset

add fee asset to UnsignedTransaction

a354e8c

update execution to deduct fee from specified fee asset

074ae0d

comment

f218e22

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

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

fix tests

53e6410

github-actions bot added sequencer-relayer pertaining to the astria-sequencer-relayer crate composer pertaining to composer labels Nov 9, 2023

noot temporarily deployed to BUF November 9, 2023 18:01 — with GitHub Actions Inactive

lint

f3c5f1d

noot temporarily deployed to BUF November 9, 2023 19:37 — with GitHub Actions Inactive

add native_asset_base_denomination to genesis

71af6ee

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

noot added 2 commits November 9, 2023 18:34

unit test for transfers

5b79479

fix test

70e0357

noot temporarily deployed to BUF November 9, 2023 23:36 — with GitHub Actions Inactive

small proto cleanup

642e9f2

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

cleanup

e6d4323

noot marked this pull request as ready for review November 9, 2023 23:47

noot requested review from SuperFluffy and joroshiba as code owners November 9, 2023 23:47

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

noot requested review from itamarreif, sambukowski and steezeburger November 9, 2023 23:52

noot added 2 commits November 17, 2023 11:03

finish addressing comments and cleanup nria usage

924d89d

lint

5466029

noot requested a review from SuperFluffy November 17, 2023 16:04

noot temporarily deployed to BUF November 17, 2023 16:07 — with GitHub Actions Inactive

SuperFluffy reviewed Nov 20, 2023

View reviewed changes

SuperFluffy approved these changes Nov 20, 2023

View reviewed changes

noot added 3 commits November 20, 2023 12:47

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

86b7f56

…r-asset

address comments

61fda91

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

7db9b4b

…r-asset

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

noot added 2 commits November 20, 2023 13:19

commit protos

03bff36

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

8503ffd

…r-asset

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

noot merged commit 406dd23 into main Nov 20, 2023

noot deleted the noot/sequencer-asset branch November 20, 2023 20:18

jbowen93 mentioned this pull request Nov 28, 2023

chore: upgrading to latest release versions astriaorg/dev-cluster#130

Merged

feat(sequencer): implement support for arbitrary assets #568

feat(sequencer): implement support for arbitrary assets #568

Uh oh!

Conversation

noot commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

Uh oh!

SuperFluffy Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noot Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

noot commented Nov 8, 2023 •

edited

Loading

SuperFluffy Nov 20, 2023 •

edited

Loading

SuperFluffy Nov 20, 2023 •

edited

Loading

SuperFluffy Nov 20, 2023 •

edited

Loading

noot Nov 20, 2023 •

edited

Loading