Use a lazy constructor to avoid allocation on every single call (i.e. even if you don't end up hitting the error case):

Why allocate a new string prior to removing strip_prefix? Because one sometimes wants to remove a prefix that reaches "into" the denom part?

Nomenclature wise this leaves me pretty confused. Just looking at the definition of Denom I see a prefix and a denom. So why would a method called remove_prefix also remove parts of the Denom?

the Denom consists of a prefix and a base denom (I'll rename the denom field to base_denom), this is just a "denomination trace" broken down into parts (eg. transfer/channel-1/utia is broken down into transfer/channel-1 and utia). so removing a prefix is removing some prefix from the entire denomination trace. I'll change this to just remove from the denom.prefix field as I don't think there's ever a case we need to go into the base denom

Can you add unit tests for this functionality?

I believe this function is fundamentally broken and very hard to fix. I have refactored denom parsing here: #1162

Context:

Assume I have this input "some/denomination/trace", then Denom::from("some/denomination/trace") will give me:

my_denom := Denom {
    prefix: "some/denomination",
    trace: "trace",
}

Now, my_denom.is_prefix_by("some/denom") == true. But even worse, I can get this:

stripped_denom
    := my_denom.remove_prefix("some/denom")
     = Denom {
    prefix: "ination",
    trace: "trace",
}

Is this actually correct?

This is the definition of fn is_prefixed (after the changes)

fn is_prefixed(source_port: &PortId, source_channel: &ChannelId, asset: &Denom) -> bool {
    let prefix = format!("{source_port}/{source_channel}/");
    asset.is_prefixed_with(&prefix)
}

But here is Asset::is_prefixed_with:

    pub fn is_prefixed_with(&self, prefix: &str) -> bool {
       self.prefix.starts_with(prefix)
    }

So this code is checking Asset::prefix if it starts with "{source_port}/{source_channel}", but it actually removing removing {source_port}/{source_channel} from the concatenation {Asset::prefix}/{Asset::denom}.

yes this is the correct functionality, for example if you have a token {source_port}/{source_channel}/base_denom then is_prefixed is true, and if this is source we want to remove {source_port}/{source_channel} from the whole denom. this is the same functionality as before, except now we handle the case where a token has multiple IBC hops, and is something like {source_port}/{source_channel}/other_port/other_channel/base_denom. previously, the entire prefix {source_port}/{source_channel}/other_port/other_channel would have been removed, which is wrong, we only want to remove the {source_port}/{source_channel} prefix, so the resulting denom is other_port/other_channel/base_denom.

I don't think we need the clone here

Should we maybe check at this point that the stripped prefix is either empty or starts with / and return an error if not? Just now this would allow stripping e.g. p0/p from p0/p1 leaving 1 which probably isn't desirable?

Either way - I guess a test for that case would be useful.

I believe this function is fundamentally broken and very hard to fix. I have refactored denom parsing here: #1162

Context:

Assume I have this input "some/denomination/trace", then Denom::from("some/denomination/trace") will give me:

my_denom := Denom {
    prefix: "some/denomination",
    trace: "trace",
}

Now, my_denom.is_prefix_by("some/denom") == true. But even worse, I can get this:

stripped_denom
    := my_denom.remove_prefix("some/denom")
     = Denom {
    prefix: "ination",
    trace: "trace",
}