refactor(core): parse ics20 denoms as ibc or trace prefixed variants by SuperFluffy · Pull Request #1181 · astriaorg/astria

SuperFluffy · 2024-06-13T13:32:32Z

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 · 2024-06-13T14:41:04Z

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

-        .channel()
+        .as_trace_prefixed()
+        .and_then(TracePrefixed::last_channel)
        .ok_or_eyre("denom must have a channel to be withdrawn via IBC")?;


Noticing this: the service does not actually enforce any form of the provided ics20 denom otherwise, but this error seems to suggest that ibc/<hash> denoms are not valid?

If that's the case the watcher builder can just parse the config input as TracePrefixed to test this at startup.

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

SuperFluffy · 2024-06-13T14:43:16Z

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

    let recipient = Address::try_from_bech32m(&recipient).context("invalid recipient address")?;

-    let is_prefixed = is_prefixed(source_port, source_channel, &unprefixed_denom);
+    let is_prefixed = denom_trace.has_exact_path(&format!("{source_port}/{source_channel}"));


As above, is the exact match maybe too strict? Would it be enough to check if it just starts with {source_port}/{source_channel}?

yeah, should just check the start

As above, replaced TracePrefixed::has_exact_path replaced by TracePrefixed::starts_with_str in 010b7e

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

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

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

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

noot

overall the changes look good! but have a few comments on the logic that should be addressed.

noot

looks good to me!

* main: chore(bridge-withdrawer): add missing errors and clean up names (#1178) feat(sequencer): add ttl and invalid cache to app mempool (#1138) chore(astria-merkle): add benchmarks (#1179) chore(sequencer-relayer): add timeout to gRPCs to Celestia app (#1191) refactor(core): parse ics20 denoms as ibc or trace prefixed variants (#1181) Mycodecrafting/sequencer seed node (#1188) chore: register all metrics during startup (#1144) feat(charts): option to purge geth mempool (#1182)

) ## Summary Replaces the bare asset ID bytes by full IBC ICS20 denoms. ## Background Bare denom IDs in public APIs are difficult to understand. While IBC ICS20 denoms are prescribed to either contain strings of the form `port/channel/denom` or `ibc/<sha256-hex>`, due to protobuf-to-json mapping protobuf `bytes` are always encoded as base64. With #1181 we parse denoms into either of the two forms and can thus allow users to provide assets as protobuf `string`s which are then parsed by sequencer. In addition, since denominations of the form `port/channel/denom` can always be converted to `ibc/<sha256>`, this allows for more elegant fetching of denom-related data from storage. ## Changes - Replace protobuf message fields from bytes to string (mostly in transaction/action types): - `bytes asset_id -> string asset` - `bytes fee_asset_id -> string fee_asset` - `repeated bytes fee_asset_ids -> repeated string fee_assets` (and rename `AllowedFeeAssetIdsResponse` to `AllowedFeeAsssetsRersponse`) - Update all affected checked `astria-core` types to parse the asset strings to `astria_core::primitive::v1::asset::Denom`s instead of `[u8; 32]` arrays. - Remove the `astria_core::primitive::v1::asset::Id` type - Remove all notion of a default native asset type from astria_core (all services and tests are now expected to have their own notion of asset types; either through config/genesis or by defining their tests accordingly) - Update all sequencer `StateWriteExt` and `StateWriteRead` types that read or write assets to disk: - instead of writing `asset::Id`s (in practice `[u8; 32]`), they now write `asset::IbcPrefixed` - make all trait methods generic over a parameter `TAsset: Into<asset::IbcPrefixed>` which allows fetching data using `astria::Denom`s, `astria::TracePrefixed`, or `astria::IbcPrecixed` - update all storage keys that used IDs to use ibc-prefixed denoms instead - provide snapshot tests for all such storage keys - update all `astria-cli` subcommands that construct actions with assets to have arguments `--asset` or `--fee-asset` (using `"nria"` as the default if not provided) - update astria-composer to have an config env var `ASTRIA_COMPOSER_FEE_ASSET` ## Testing All tests that involve fees were updated and pass again. ## Breaking Changelist - This is a network breaking change because all transaction/action messages that took bytes now take strings

…209) ## Summary Replaces the bare asset ID bytes by full IBC ICS20 denoms. ## Background Bare denom IDs in public APIs are difficult to understand. While IBC ICS20 denoms are prescribed to either contain strings of the form `port/channel/denom` or `ibc/<sha256-hex>`, due to protobuf-to-json mapping protobuf `bytes` are always encoded as base64. With astriaorg/astria#1181 we parse denoms into either of the two forms and can thus allow users to provide assets as protobuf `string`s which are then parsed by sequencer. In addition, since denominations of the form `port/channel/denom` can always be converted to `ibc/<sha256>`, this allows for more elegant fetching of denom-related data from storage. ## Changes - Replace protobuf message fields from bytes to string (mostly in transaction/action types): - `bytes asset_id -> string asset` - `bytes fee_asset_id -> string fee_asset` - `repeated bytes fee_asset_ids -> repeated string fee_assets` (and rename `AllowedFeeAssetIdsResponse` to `AllowedFeeAsssetsRersponse`) - Update all affected checked `astria-core` types to parse the asset strings to `astria_core::primitive::v1::asset::Denom`s instead of `[u8; 32]` arrays. - Remove the `astria_core::primitive::v1::asset::Id` type - Remove all notion of a default native asset type from astria_core (all services and tests are now expected to have their own notion of asset types; either through config/genesis or by defining their tests accordingly) - Update all sequencer `StateWriteExt` and `StateWriteRead` types that read or write assets to disk: - instead of writing `asset::Id`s (in practice `[u8; 32]`), they now write `asset::IbcPrefixed` - make all trait methods generic over a parameter `TAsset: Into<asset::IbcPrefixed>` which allows fetching data using `astria::Denom`s, `astria::TracePrefixed`, or `astria::IbcPrecixed` - update all storage keys that used IDs to use ibc-prefixed denoms instead - provide snapshot tests for all such storage keys - update all `astria-cli` subcommands that construct actions with assets to have arguments `--asset` or `--fee-asset` (using `"nria"` as the default if not provided) - update astria-composer to have an config env var `ASTRIA_COMPOSER_FEE_ASSET` ## Testing All tests that involve fees were updated and pass again. ## Breaking Changelist - This is a network breaking change because all transaction/action messages that took bytes now take strings

refactor(core): parse ics20 denoms

1519139

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

fmt

7d0c252

SuperFluffy marked this pull request as ready for review June 13, 2024 14:37

SuperFluffy requested a review from a team as a code owner June 13, 2024 14:37

SuperFluffy requested a review from Fraser999 June 13, 2024 14:37

SuperFluffy commented Jun 13, 2024

View reviewed changes

crates/astria-sequencer/src/ibc/ics20_transfer.rs Outdated Show resolved Hide resolved

SuperFluffy commented Jun 13, 2024

View reviewed changes

SuperFluffy changed the title ~~refactor(core): parse ics20 denoms~~ refactor(core): parse ics20 denoms as ibc or trace prefixed variants Jun 13, 2024

SuperFluffy requested a review from noot June 13, 2024 14:44