fix(core, sequencer): prefix removal source non-refund ics20 packet #1162

SuperFluffy · 2024-06-07T14:30:18Z

Summary

Rewrites the IBC denomation construction by parsing its prefix into segments. This fixes the incorrect removal of prefxies for source non-refund ics20 packets.

Background

follow up to #851 which was an audit fix - this fixes the fix by removing the source prefix correctly, in the case assets we're the source of are transferred in. previously, the entire prefix was removed, would would have been invalid if the asset was prefixed by multiple hops, eg. transfer/channel-1/transfer/channel-2/denom would have become just denom when it should have become transfer/channel-2/denom.

Changes

refactor astria_core::primitive::1::asset::Denom to express its prefix as a list of segments
ensure that only full prefix segments are removed
make Denom construction a fallible operation by parsing it
removed a bunch of constructors as they were only used in tests and not otherwise

Testing

Provide exhaustive units.

Related Issues

Replaces #1131

SuperFluffy · 2024-06-07T15:05:00Z

crates/astria-bridge-withdrawer/src/withdrawer/ethereum/convert.rs

-        .prefix()
-        .rsplit_once('/')
+    let channel = denom
+        .channel()


Instead of doing this ad-hoc I created a method for this.

SuperFluffy · 2024-06-07T15:05:48Z

crates/astria-core/src/primitive/v1/asset.rs

-    pub fn prefix(&self) -> &str {
-        &self.prefix
+    pub fn channel(&self) -> Option<&str> {
+        if self.prefix_segments.len() < 2 {


I am not sure if this is always true. But I went with how this was set in astria-bridge-withdrawer which just seems to take the last segment, if present.

this should be the second prefix segment actually (the channel closest to the start of the denom trace)

SuperFluffy · 2024-06-07T15:07:55Z

crates/astria-core/src/primitive/v1/asset.rs

+        // For the current implementation, the number of slashes in a prefix is equal to the number
+        // of segments + 1.
+        // So a "path/to" would be a prefix to `"path/to/denom"`, but `"path/to/"` would not
+        let number_of_slashes = prefix.chars().filter(|c| *c == '/').count();


This just short-circuits to avoid having to walk over the entire slice.

SuperFluffy · 2024-06-07T15:11:19Z

crates/astria-core/src/primitive/v1/asset.rs

+        .id()
+}
+
+struct DenomFmt<'a> {


We use this for constructing the id ad-hoc and for formatting.

SuperFluffy · 2024-06-07T15:12:27Z

crates/astria-core/src/primitive/v1/asset.rs

@@ -137,6 +252,13 @@ impl Id {
        Self(hash.into())
    }

+    fn from_denom_fmt(denom_fmt: &DenomFmt<'_>) -> Self {


This is just a quick fix and should be replaced by a non-allocating implementation.

Also, the Id::from_denom above is should actually take a Denom, not a str.

Fraser999

LGTM - just a few nitpicks.

crates/astria-core/src/protocol/account/v1alpha1/mod.rs

crates/astria-core/src/protocol/asset/v1alpha1/mod.rs

crates/astria-core/src/primitive/v1/asset.rs

* main: fix: ignore RUSTSEC-2021-0139 (#1171) chore(sequencer-relayer)!: remove functionality to restrict relaying blocks to only those proposed by a given validator (#1168) chore(metrics): update `metric_name` macro to handle a collection of names (#1163) fix(bridge-withdrawer): skip linting generated contract code (#1172) fix(core, sequencer): prefix removal source non-refund ics20 packet (#1162) chore(docs): add sequencer-relayer doc to specs (#1126) feat(bridge-withdrawer): sync logic (#1165) chore(withdrawer): replace contracts with `astria-bridge-contracts` submodule (#1164) feat(sequencer)!: implement bridge sudo and withdrawer addresses (#1142) feat(sequencer): implement refund to rollup logic upon ics20 transfer refund (#1161) feat(bridge-withdrawer): bridge withdrawer startup (#1160) feat(core, proto)!: add bech32m addresses (#1124) feat(withdrawer): bridged ERC20 token withdrawals (#1149) feat(sequencer-relayer)!: add chain IDs for sequencer and Celestia to config env vars (#1063) test(bridge-withdrawer): add submitter tests (#1133) chore: bump penumbra deps (#1159) feat(sequencer): implement `bridge/account_last_tx_hash` abci query (#1158) fix(withdrawer): use block subscription in batcher; send to destination_chain_address (#1157) fix(withdrawer): update AstriaWithdrawer to check that withdrawal value is sufficient (#1148) chore(ci): build bridge withdrawer images (#1156)

…1181) ## Summary Rewrites the ics20 parsing logic to distinguish between `ibc/` and `<path>` prefixed denoms. ## Background While #1162 was an improvement of what we had before, it was still problematic in that the parsing logic did not distinguish between denom of the form `[port/channel...]/base` and `ibc/hash`, so that a lot of the logic in sequencer was done ad-hoc. This patch actually distinguishes between both forms, allowing sequencer to enforce which type of denom it stores in its database, and which type it processes further. ## Changes - Rewrite `astria_core::primitive::v1::Denom` as an enum with variants `TracePrefixed` and `IbcPrefixed` ## Testing Adapt and expands unit tests, update sequencer tests.

SuperFluffy requested a review from a team as a code owner June 7, 2024 14:30

SuperFluffy requested a review from noot June 7, 2024 14:30

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Jun 7, 2024

SuperFluffy mentioned this pull request Jun 7, 2024

fix(sequencer): fix prefix removal when receiving source non-refund ics20 packet #1131

Closed

SuperFluffy requested a review from Fraser999 June 7, 2024 15:00

SuperFluffy changed the title ~~refactor(core): stricter IBC denomination parsing~~ fix(core, sequencer): prefix removal source non-refund ics20 packet Jun 7, 2024

SuperFluffy commented Jun 7, 2024

View reviewed changes

Fraser999 approved these changes Jun 7, 2024

View reviewed changes

rewrite denomination in terms of prefixes

8b16f74

SuperFluffy force-pushed the superfluffy/ibc-prefix branch from 866ed6a to 8b16f74 Compare June 10, 2024 12:36

SuperFluffy added 2 commits June 10, 2024 15:09

fix after rebase

632be72

address review comments

2911265

SuperFluffy requested a review from a team as a code owner June 10, 2024 13:27

github-actions bot added the composer pertaining to composer label Jun 10, 2024

SuperFluffy added 3 commits June 10, 2024 15:30

doc comment

8390b4b

more review comments

53e852b

clippy

08cb666

SuperFluffy enabled auto-merge June 10, 2024 15:12

SuperFluffy added this pull request to the merge queue Jun 10, 2024

Merged via the queue into main with commit 6c29d39 Jun 10, 2024
38 checks passed

SuperFluffy deleted the superfluffy/ibc-prefix branch June 10, 2024 15:26

SuperFluffy mentioned this pull request Jun 13, 2024

refactor(core): parse ics20 denoms as ibc or trace prefixed variants #1181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core, sequencer): prefix removal source non-refund ics20 packet #1162

fix(core, sequencer): prefix removal source non-refund ics20 packet #1162

SuperFluffy commented Jun 7, 2024 •

edited

Loading

SuperFluffy Jun 7, 2024

SuperFluffy Jun 7, 2024 •

edited

Loading

noot Jun 10, 2024

SuperFluffy Jun 7, 2024

SuperFluffy Jun 7, 2024

SuperFluffy Jun 7, 2024

Fraser999 left a comment

fix(core, sequencer): prefix removal source non-refund ics20 packet #1162

fix(core, sequencer): prefix removal source non-refund ics20 packet #1162

Conversation

SuperFluffy commented Jun 7, 2024 • edited Loading

Summary

Background

Changes

Testing

Related Issues

SuperFluffy Jun 7, 2024

Choose a reason for hiding this comment

SuperFluffy Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

noot Jun 10, 2024

Choose a reason for hiding this comment

SuperFluffy Jun 7, 2024

Choose a reason for hiding this comment

SuperFluffy Jun 7, 2024

Choose a reason for hiding this comment

SuperFluffy Jun 7, 2024

Choose a reason for hiding this comment

Fraser999 left a comment

Choose a reason for hiding this comment

SuperFluffy commented Jun 7, 2024 •

edited

Loading

SuperFluffy Jun 7, 2024 •

edited

Loading