Skip to content

use an address length limit compatible with ibc-go #1674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

dynst
Copy link
Contributor

@dynst dynst commented Jun 17, 2025

ibc-go 8.0 added length validation that an address is a 2048 byte string or smaller - before that there was no maximum length. Make the faucet's isValidAddress function match it.

https://github.com/cosmos/ibc-go/blob/904047fc783f43ab87e3121c2c0c47ef7a6b075e/modules/apps/transfer/types/msgs.go#L16

cosmos/ibc-go#4859 (comment)

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to do this change? 2048 is just a safety measure to avoid too much data.

We can set a limit in fromBech32 if that is needed.

As address lengths only 32 and 20 bytes is used in practice. So we need different data lengths for something?

@dynst
Copy link
Contributor Author

dynst commented Jun 17, 2025

As address lengths only 32 and 20 bytes is used in practice.

That hasn't been true for a while now; chains that use novel cryptographic algorithms use longer lengths, like Penumbra. There was a whole compatibility issue with Noble enforcing a maximum address string length of 90 bytes (because they used a BIP-172 compliant bech32 library).

circlefin/noble-fiattokenfactory#32

Penumbra has both Bech32m addresses and Bech32 ones for compatibility, and Bech32 addresses can be 150 characters long (e.g. penumbracompat11ld2kghffzgwq4597ejpgmnwxa7ju0cndytuxtsjh8qhjyfuwq0rwd5flnw4a3fgclw7m5puh50nskn2c88flhne2hzchnpxru609d5wgmqqvhdf0sy2tktqfcm2p2tmxeuc86n).

https://forum.penumbra.zone/t/usdc-transfers-temporarily-affected-by-noble-chain-upgrade/129

https://protocol.penumbra.zone/main/addresses_keys/addresses.html#address-encodings

@webmaster128
Copy link
Member

I see. Then we can fix it like this:

diff --git a/packages/faucet/src/addresses.spec.ts b/packages/faucet/src/addresses.spec.ts
index 639f52970d..c5c2725425 100644
--- a/packages/faucet/src/addresses.spec.ts
+++ b/packages/faucet/src/addresses.spec.ts
@@ -11,6 +11,12 @@ describe("isValidAddress", () => {
     );
   });
 
+  it("accepts an Penumbra address", () => {
+    expect(isValidAddress("penumbracompat11ld2kghffzgwq4597ejpgmnwxa7ju0cndytuxtsjh8qhjyfuwq0rwd5flnw4a3fgclw7m5puh50nskn2c88flhne2hzchnpxru609d5wgmqqvhdf0sy2tktqfcm2p2tmxeuc86n", "penumbracompat1")).toBe(
+      true,
+    );
+  });
+
   it("rejects an invalid address", () => {
     expect(isValidAddress("cosmos1fail", "cosmos")).toBe(false);
   });
diff --git a/packages/faucet/src/addresses.ts b/packages/faucet/src/addresses.ts
index e336322689..36eeb986b7 100644
--- a/packages/faucet/src/addresses.ts
+++ b/packages/faucet/src/addresses.ts
@@ -1,12 +1,16 @@
 import { fromBech32 } from "@cosmjs/encoding";
 
+// Penumbra are up to 150 chars. ibg-go has a limit of 2048.
+// See https://github.com/cosmos/cosmjs/pull/1674
+const lengthLimit = 512;
+
 export function isValidAddress(input: string, requiredPrefix: string): boolean {
   try {
-    const { prefix, data } = fromBech32(input);
+    const { prefix, data } = fromBech32(input, lengthLimit);
     if (prefix !== requiredPrefix) {
       return false;
     }
-    return data.length >= 20 && data.length <= 32;
+    return data.length >= 20 && data.length <= 128;
   } catch {
     return false;
   }

@webmaster128
Copy link
Member

Merged as part of #1683. Sorry, I missed the last commit now. But did not have rights to push to the PR for some reason.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants