[Feat] Fix xrpl interface by tanut32039 · Pull Request #15 · bandprotocol/fkms

tanut32039 · 2026-02-27T07:06:43Z

Add xrpl sign logic

refactor tx encode/decode to codec dir
add tss signature verifier for xrpl signing
add DERSIgner

:

Copilot

Pull request overview

This pull request adds XRPL (XRP Ledger) signing support to the FKMS (Falcon Key Management Service) alongside existing EVM functionality. The changes introduce TSS (Threshold Signature Scheme) signature verification, XRPL-specific transaction encoding, and extend the gRPC API with new RPC methods for XRPL signing. The implementation includes codec modules for decoding TSS messages (with tick and fixed-point price encoding), EVM transactions, and XRPL transaction construction.

Changes:

Added TSS signature verification system with challenge-based verification for secure transaction signing
Implemented XRPL signing support with DER signature format and compressed public key handling
Extended proto interface with SignXrpl RPC method and supporting message types for XRPL transactions

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`src/verifier/tss/signature.rs`	New TSS signature verifier using Schnorr-like verification with challenge computation
`src/verifier/tss/message.rs`	TSS message verification comparing prices from transaction and TSS data
`src/codec/tss.rs`	TSS message decoder supporting fixed-point and tick-based price encoding
`src/codec/evm.rs`	EVM transaction decoder extracting TSS data from legacy and EIP-1559 transactions
`src/codec/xrpl.rs`	XRPL transaction encoder creating OracleSet transactions with price data
`src/signer.rs`	Added XrplSigner trait and XRPL address derivation using RIPEMD-160
`src/signer/local.rs`	Extended LocalSigner to support both ECDSA and DER signatures with compressed keys
`src/signer/aws.rs`	Updated AWS signer interface to support is_compressed parameter
`src/signer/signature/ecdsa.rs`	Added DerSignature type for XRPL signing
`src/server/service.rs`	New unified service implementation with sign_evm and sign_xrpl endpoints
`src/server/builder.rs`	Extended builder to support XRPL signers and TSS verifier
`src/server.rs`	Updated server struct to hold both EVM and XRPL signers
`src/server/pre_sign.rs`	Changed PreSignHook interface to accept prices instead of raw message
`src/config/signer/local.rs`	Added ChainType enum to distinguish EVM and XRPL configurations
`src/commands/utils.rs`	Separated signer loading logic for EVM (coin_type 60) and XRPL (coin_type 144)
`src/commands/start.rs`	Added TSS public key loading from environment variable
`src/commands/key.rs`	Updated key display to show both EVM and XRPL addresses
`proto/fkms/v1/signer.proto`	Added SignXrpl RPC method and supporting message types
`Cargo.toml`	Added dependencies for XRPL support including xrpl-rust and xrpl_binary_codec
`.github/workflows/ci.yml`	Updated Rust toolchain version
`README.md`	Updated documentation to reflect XRPL support

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (1)

src/signer/aws.rs:72

AwsSigner::sign_ecdsa_internal uses SigningAlgorithmSpec::EcdsaSha256 without setting message_type to DIGEST, so AWS KMS will hash the provided bytes with SHA-256 before signing. The new Signer::sign_ecdsa interface is being called with a prehash (Keccak256 digest) in server/service.rs, so the AWS signer will produce signatures over SHA256(keccak(tx)) while the local signer signs keccak(tx) directly. Align AWS signing behavior with the prehash contract (e.g., set message_type appropriately and/or adjust what is passed to KMS).

        let sign_output = self
            .client
            .sign()
            .key_id(&self.key_id)
            .message(Blob::new(message))
            .signing_algorithm(aws_sdk_kms::types::SigningAlgorithmSpec::EcdsaSha256)
            .send()
            .await?;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T09:36:37Z

@@ -52,35 +73,48 @@ impl AwsSigner {
            .send()
            .await?;

-        let signature_blob = sign_output
-            .signature
-            .ok_or(anyhow::Error::msg("no signature found"))?;
+        let signature_blob = sign_output.signature.ok_or(anyhow!("no signature found"))?;

-        let signature = ecdsa::Signature::from_der(signature_blob.as_ref())?;
+        match self.chain_type {
+            ChainType::Evm => {
+                let signature = ecdsa::Signature::from_der(signature_blob.as_ref())?;
+                let digest = Keccak256::new_with_prefix(message);
+                let recovery_id = self.find_recovery_id(digest, &signature)?;
+                let recoverable_signature: EcdsaSignature = (signature, recovery_id);
+                Ok(recoverable_signature.into_vec())
+            }
+            _ => Err(anyhow!("Unsupported Chain Type")),
+        }


AwsSigner::sign sends message directly to KMS with SigningAlgorithmSpec::EcdsaSha256 but does not set message_type = DIGEST. In the server, message is already a 32-byte Keccak256 digest, so KMS will hash it with SHA-256 again, producing an invalid signature for EVM usage. Also, recovery-id search re-hashes message via Keccak256::new_with_prefix(message); if message is already a digest, recovery should use a prehash recovery API (or use the exact digest bytes that were signed).

Copilot · 2026-03-11T09:36:37Z

+pub fn public_key_to_evm_address(public_key: &[u8]) -> anyhow::Result<String> {
+    // Check exact length
+    if public_key.len() != 65 {
+        return Err(anyhow!(
+            "Invalid public key length for EVM address. Expected 65 bytes, got {}",
+            public_key.len()
+        ));
    }
-}

-fn public_key_to_evm_address(public_key: &[u8]) -> String {
    let mut hasher = sha3::Keccak256::new();
-    hasher.update(&public_key[1..]); // Skip the first byte (0x04)
-    let hash = hasher.finalize().to_vec();
-    format!("0x{}", hex::encode(&hash[12..]))
+
+    // We can now safely skip the first byte because we validated length and prefix
+    hasher.update(&public_key[1..]);
+    let hash = hasher.finalize();


public_key_to_evm_address only validates the key length, but the comment says the prefix is validated and the implementation always skips public_key[0]. If a non-uncompressed SEC1 key (or wrong prefix) is passed, this will silently compute the wrong address. Consider validating public_key[0] == 0x04 (and returning an error otherwise) or explicitly accepting both compressed/uncompressed forms and normalizing first.

Copilot · 2026-03-11T09:36:37Z

+    async fn test_sign_der() {
+        let pk = hex::decode("45ea1ad910df93d1a021a42f384aad55c7c65d565ad6b2203a4ba50418922a7b")
+            .unwrap();
+        let signer = LocalSigner::new(&pk, &ChainType::Evm).unwrap();


test_sign_der constructs the signer with ChainType::Evm, but DER signing is only used for XRPL in LocalSigner::sign. This makes the test inconsistent with the production behavior and could hide regressions in XRPL-specific setup (compressed pubkey/address). Update the test to use ChainType::Xrpl for the signer instance.

Suggested change

let signer = LocalSigner::new(&pk, &ChainType::Evm).unwrap();

let signer = LocalSigner::new(&pk, &ChainType::Xrpl).unwrap();

Copilot · 2026-03-11T09:36:38Z

@@ -139,6 +179,50 @@
 }
 ```


README's GetSignerAddressesResponse example still shows repeated string addresses, but the proto was changed to repeated Signers signers. This is misleading for API consumers; update the example to match proto/fkms/v1/signer.proto.

Copilot · 2026-03-11T09:36:38Z

+    // Convert characters to Hex uppercase
+    let mut hex_str = s.chars().fold(String::new(), |mut acc, c| {
+        acc.push_str(&format!("{:02X}", c as u32));
+        acc
+    });
+


str_to_hex claims to only support ASCII, but it operates on chars without enforcing is_ascii(). Non-ASCII input will produce variable-length hex (e.g., >2 bytes per char), potentially violating XRPL field-size expectations and breaking encoding. Consider validating s.is_ascii() and encoding via s.as_bytes() instead of iterating chars.

Suggested change

// Convert characters to Hex uppercase

let mut hex_str = s.chars().fold(String::new(), |mut acc, c| {

acc.push_str(&format!("{:02X}", c as u32));

acc

});

// Enforce ASCII-only input to avoid variable-length and invalid encodings

if !s.is_ascii() {

return Err(anyhow!(

"str_to_hex only supports ASCII input, got non-ASCII string: {}",

s

));

}

// Convert bytes to uppercase hex (two hex chars per byte)

let mut hex_str = String::new();

for b in s.as_bytes() {

hex_str.push_str(&format!("{:02X}", b));

}

Copilot · 2026-03-11T09:36:38Z

 [[signer_config.local_signer_configs]]
 type = "private_key"
 env_variable = "PRIVATE_KEY_1"
 encoding = "hex"
+chain_type = "evm"

 [[signer_config.local_signer_configs]]
 type         = "mnemonic"
 env_variable = "MNEMONIC_1"
 coin_type = 60
 account = 0
 index = 0
+
+[tss]
+enable_verify = true
+
+[[tss.groups]]
+public_key = "0232a786de4c99679f10fb9a1c94d17737739e413bef385a2f8a26f901a40fdc8b"
+expired_time = 1234567890
 ```

 ### Supported Local Signer Types

 | Type          | Description                                    | Required Fields                                |
 | --------------| ---------------------------------------------- | -----------------------------------------------|
-| `private_key` | Load private key from an environment variable  | `env_variable`, `encoding`                     |
+| `private_key` | Load private key from an environment variable (Currently support only EVM) | `env_variable`, `encoding`                     |
 | `mnemonic`    | Load mnemonic from an environment variable     | `env_variable`, `coin_type`, `account`, `index`|



The sample config.toml is missing chain_type for the mnemonic signer config, but the code requires it. Also, the table says private_key currently supports only EVM, but LocalSigner::new supports both EVM and XRPL based on chain_type. Please update the example/table to match the actual configuration requirements and supported chains.

Tanut Lertwarachai added 12 commits February 9, 2026 01:43

add xrpl

b1d1cf2

add unit test

6e56f48

add comment

d9860b2

fix log

26b08bd

fix cin failed

490c528

fix copilot comment

14df31c

fix lint

a24a080

fix some copilot comment

a36d1be

fix copilot comment

133a67c

:

fix interface

4e4a1f4

fix bug tx type

63670fe

fix ci

5db0e4e

tanut32039 requested review from Kissadada, Marca23 and RogerKSI February 27, 2026 07:06

Tanut Lertwarachai added 2 commits February 27, 2026 14:31

update readme

a24a373

delete print

8c6292d

RogerKSI requested a review from Copilot February 27, 2026 10:44

Copilot started reviewing on behalf of RogerKSI February 27, 2026 10:45 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Marca23 reviewed Mar 5, 2026

View reviewed changes

Comment thread src/codec/xrpl.rs

Comment thread src/commands/start.rs

Comment thread src/commands/utils.rs Outdated

Comment thread src/signer/local.rs Outdated

Comment thread src/server/service.rs Outdated

Comment thread src/server/service.rs Outdated

Marca23 reviewed Mar 5, 2026

View reviewed changes

Comment thread src/server/service.rs Outdated

fix comment

f312a57

RogerKSI reviewed Mar 6, 2026

View reviewed changes

Comment thread README.md

Comment thread src/server.rs Outdated

delete Signal field

15d4bcd

RogerKSI requested review from Marca23, RogerKSI and Copilot March 9, 2026 15:34

Copilot started reviewing on behalf of RogerKSI March 9, 2026 15:35 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

Marca23 approved these changes Mar 10, 2026

View reviewed changes

Tanut Lertwarachai added 3 commits March 10, 2026 16:26

use multi public key

6675d5b

change signer trait

6be036d

fix bug

a765313

RogerKSI requested a review from Copilot March 10, 2026 17:39

Copilot started reviewing on behalf of RogerKSI March 10, 2026 17:40 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

RogerKSI reviewed Mar 10, 2026

View reviewed changes

Comment thread proto/fkms/v1/signer.proto

Comment thread src/signer/local.rs Outdated

Comment thread src/signer/aws.rs

Comment thread src/signer.rs Outdated

Tanut Lertwarachai added 2 commits March 11, 2026 11:10

fix comment

a0108fc

merge chain type in localsigner

77e55a0

RogerKSI requested review from Marca23, RogerKSI and Copilot March 11, 2026 07:41

Copilot started reviewing on behalf of RogerKSI March 11, 2026 07:42 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

RogerKSI reviewed Mar 11, 2026

View reviewed changes

Comment thread README.md Outdated

combine merge logic

bdba228

RogerKSI requested a review from Copilot March 11, 2026 08:47

Copilot started reviewing on behalf of RogerKSI March 11, 2026 08:48 View session

delete matching chain type

375faa7

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Comment thread src/verifier/tss/signature.rs Outdated

Comment thread src/signer.rs

Comment thread README.md

Comment thread src/verifier/tss/signature.rs Outdated

Comment thread src/signer/aws.rs

Comment thread README.md

Comment thread src/server/service.rs Outdated

Tanut Lertwarachai added 2 commits March 11, 2026 16:06

fix readme

6e08e2b

fix error message

2471c0b

RogerKSI approved these changes Mar 11, 2026

View reviewed changes

RogerKSI requested a review from Copilot March 11, 2026 09:23

Copilot started reviewing on behalf of RogerKSI March 11, 2026 09:23 View session

Marca23 approved these changes Mar 11, 2026

View reviewed changes

RogerKSI merged commit 3e1d40b into main Mar 11, 2026
5 checks passed

RogerKSI deleted the fix-xrpl-interface branch March 11, 2026 09:33

Copilot AI reviewed Mar 11, 2026

View reviewed changes

	let signer = LocalSigner::new(&pk, &ChainType::Evm).unwrap();
	let signer = LocalSigner::new(&pk, &ChainType::Xrpl).unwrap();

-    // Convert characters to Hex uppercase
-    let mut hex_str = s.chars().fold(String::new(), |mut acc, c| {
-        acc.push_str(&format!("{:02X}", c as u32));
-        acc
-    });
+    // Enforce ASCII-only input to avoid variable-length and invalid encodings
+    if !s.is_ascii() {
+        return Err(anyhow!(
+            "str_to_hex only supports ASCII input, got non-ASCII string: {}",
+            s
+        ));
+    }
+    // Convert bytes to uppercase hex (two hex chars per byte)
+    let mut hex_str = String::new();
+    for b in s.as_bytes() {
+        hex_str.push_str(&format!("{:02X}", b));
+    }

Conversation

tanut32039 commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tanut32039 commented Feb 27, 2026 •

edited

Loading