From 49116ab11aa62d76271d6ea1121d0f47c6db4b95 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Mon, 12 May 2025 17:12:10 -0400 Subject: [PATCH 01/19] feat: implement pallet_msa::withdraw_tokens extrinsic --- Cargo.lock | 1 + e2e/msa/msaTokens.test.ts | 174 +++++++++++++++++++++++++++- e2e/scaffolding/extrinsicHelpers.ts | 12 ++ e2e/scaffolding/helpers.ts | 2 +- pallets/msa/Cargo.toml | 1 + pallets/msa/src/lib.rs | 120 ++++++++++++++++++- pallets/msa/src/types.rs | 6 +- pallets/msa/src/weights.rs | 15 +++ runtime/frequency/src/lib.rs | 2 + 9 files changed, 323 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0a5a48972d..67f5501bed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7862,6 +7862,7 @@ dependencies = [ "hex", "lazy_static", "log", + "pallet-balances", "pallet-collective", "pallet-handles", "pallet-schemas", diff --git a/e2e/msa/msaTokens.test.ts b/e2e/msa/msaTokens.test.ts index 29c2b69033..84eb0a7d08 100644 --- a/e2e/msa/msaTokens.test.ts +++ b/e2e/msa/msaTokens.test.ts @@ -1,14 +1,24 @@ -/* eslint-disable mocha/no-skipped-tests */ import '@frequency-chain/api-augment'; import assert from 'assert'; -import { ExtrinsicHelper } from '../scaffolding/extrinsicHelpers'; -import { ethereumAddressToKeyringPair } from '../scaffolding/ethereum'; +import { AddKeyData, ExtrinsicHelper } from '../scaffolding/extrinsicHelpers'; +import { ethereumAddressToKeyringPair, getUnifiedAddress, getUnifiedPublicKey } from '../scaffolding/ethereum'; import { getFundingSource } from '../scaffolding/funding'; import { H160 } from '@polkadot/types/interfaces'; import { bnToU8a, hexToU8a, stringToU8a } from '@polkadot/util'; import { KeyringPair } from '@polkadot/keyring/types'; import { keccak256AsU8a } from '@polkadot/util-crypto'; -import { getExistentialDeposit } from '../scaffolding/helpers'; +import { + CENTS, + createAndFundKeypair, + createKeys, + DOLLARS, + generateAddKeyPayload, + getExistentialDeposit, + signPayloadSr25519, + Sr25519Signature, +} from '../scaffolding/helpers'; +import { u64 } from '@polkadot/types'; +import { Codec } from '@polkadot/types/types'; const fundingSource = getFundingSource(import.meta.url); @@ -81,7 +91,7 @@ describe('MSAs Holding Tokens', function () { describe('Send tokens to MSA', function () { it('should send tokens to the MSA', async function () { - const ed = await getExistentialDeposit(); + const ed = getExistentialDeposit(); const transferAmount = 1n + ed; let accountData = await ExtrinsicHelper.getAccountInfo(ethKeys); const initialBalance = accountData.data.free.toBigInt(); @@ -103,4 +113,158 @@ describe('MSAs Holding Tokens', function () { ); }); }); + + describe('withdrawTokens', function () { + let keys: KeyringPair; + let msaId: u64; + let msaAddress: H160; + let secondaryKey: KeyringPair; + const defaultPayload: AddKeyData = {}; + let payload: AddKeyData; + let ownerSig: Sr25519Signature; + let badSig: Sr25519Signature; + let addKeyData: Codec; + + before(async function () { + // Setup an MSA with tokens + keys = await createAndFundKeypair(fundingSource, 5n * CENTS); + const { target } = await ExtrinsicHelper.createMsa(keys).signAndSend(); + assert.notEqual(target?.data.msaId, undefined, 'MSA Id not in expected event'); + msaId = target!.data.msaId; + + const { accountId } = await ExtrinsicHelper.apiPromise.call.msaRuntimeApi.getEthereumAddressForMsaId(msaId); + msaAddress = accountId; + + secondaryKey = await createAndFundKeypair(fundingSource, 5n * CENTS); + + // Default payload making it easier to test `withdrawTokens` + defaultPayload.msaId = msaId; + defaultPayload.newPublicKey = getUnifiedPublicKey(secondaryKey); + }); + + beforeEach(async function () { + payload = await generateAddKeyPayload(defaultPayload); + }); + + it('should fail if origin is not address contained in the payload (NotKeyOwner)', async function () { + const badPayload = { ...payload, newPublicKey: getUnifiedAddress(createKeys()) }; // Invalid MSA ID + addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', badPayload); + ownerSig = signPayloadSr25519(keys, addKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, badPayload); + await assert.rejects(op.fundAndSend(fundingSource), { + name: 'NotKeyOwner', + }); + }); + + it('should fail if MSA owner signature is invalid (MsaOwnershipInvalidSignature)', async function () { + addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', payload); + badSig = signPayloadSr25519(createKeys(), addKeyData); // Invalid signature + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, badSig, payload); + await assert.rejects(op.fundAndSend(fundingSource), { + name: 'MsaOwnershipInvalidSignature', + }); + }); + + it('should fail if expiration has passed (ProofHasExpired)', async function () { + const newPayload = await generateAddKeyPayload({ + ...defaultPayload, + expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber(), + }); + addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); + ownerSig = signPayloadSr25519(keys, addKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, newPayload); + await assert.rejects(op.fundAndSend(fundingSource), { + name: 'ProofHasExpired', + }); + }); + + it('should fail if expiration is not yet valid (ProofNotYetValid)', async function () { + const maxMortality = ExtrinsicHelper.api.consts.msa.mortalityWindowSize.toNumber(); + const newPayload = await generateAddKeyPayload({ + ...defaultPayload, + expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber() + maxMortality + 999, + }); + addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); + ownerSig = signPayloadSr25519(keys, addKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, newPayload); + await assert.rejects(op.fundAndSend(fundingSource), { + name: 'ProofNotYetValid', + }); + }); + + it('should fail if payload signer does not control the MSA in the signed payload (NotMsaOwner)', async function () { + const newPayload = await generateAddKeyPayload({ + ...defaultPayload, + msaId: new u64(ExtrinsicHelper.api.registry, 9999), + }); + addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); + ownerSig = signPayloadSr25519(keys, addKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, newPayload); + await assert.rejects(op.fundAndSend(fundingSource), { + name: 'NotMsaOwner', + }); + }); + + it('should fail if payload signer is not an MSA control key (NoKeyExists)', async function () { + const badSigner = createKeys(); + const newPayload = await generateAddKeyPayload({ + ...defaultPayload, + msaId: new u64(ExtrinsicHelper.api.registry, 9999), + }); + addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); + ownerSig = signPayloadSr25519(badSigner, addKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, badSigner, ownerSig, newPayload); + await assert.rejects(op.fundAndSend(fundingSource), { + name: 'NoKeyExists', + }); + }); + + it('should fail if MSA does not have a balance', async function () { + addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', payload); + ownerSig = signPayloadSr25519(keys, addKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, payload); + await assert.rejects(op.fundAndSend(fundingSource), { + name: 'InsufficientBalanceToWithdraw', + }); + }); + + it('should succeed', async function () { + // Fund receiver with known amount to pay for transaction + const startingAmount = 1n * DOLLARS; + const transferAmount = 1n * DOLLARS; + const tertiaryKeys = await createAndFundKeypair(fundingSource, startingAmount); + const { + data: { free: startingBalance }, + } = await ExtrinsicHelper.getAccountInfo(tertiaryKeys); + + // Send tokens to MSA + try { + const { target: transferEvent } = await ExtrinsicHelper.transferFunds( + fundingSource, + ethereumAddressToKeyringPair(msaAddress), + transferAmount + ).signAndSend(); + assert.notEqual(transferEvent, undefined, 'should have transferred tokens to MSA'); + } catch (err: any) { + console.error('Error sending tokens to MSA', err.message); + } + + const newPayload = await generateAddKeyPayload({ ...payload, newPublicKey: getUnifiedPublicKey(tertiaryKeys) }); + addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); + ownerSig = signPayloadSr25519(keys, addKeyData); + const op = ExtrinsicHelper.withdrawTokens(tertiaryKeys, keys, ownerSig, newPayload); + const { eventMap } = await op.fundAndSend(fundingSource); + const feeAmount = (eventMap['transactionPayment.TransactionFeePaid'].data as unknown as any).actualFee; + + // Destination account should have had balance increased + const { + data: { free: endingBalance }, + } = await ExtrinsicHelper.getAccountInfo(tertiaryKeys); + + assert( + startingBalance.toBigInt() + transferAmount - feeAmount.toBigInt() === endingBalance.toBigInt(), + 'balance of recieve should have increased by the transfer amount minus fee' + ); + }); + }); }); diff --git a/e2e/scaffolding/extrinsicHelpers.ts b/e2e/scaffolding/extrinsicHelpers.ts index 9c4cb0281b..a9652c9dd8 100644 --- a/e2e/scaffolding/extrinsicHelpers.ts +++ b/e2e/scaffolding/extrinsicHelpers.ts @@ -913,4 +913,16 @@ export class ExtrinsicHelper { ExtrinsicHelper.api.events.passkey.TransactionExecutionSuccess ); } + + public static withdrawTokens( + keys: KeyringPair, + ownerKeys: KeyringPair, + ownerSignature: MultiSignatureType, + payload: AddKeyData + ) { + return new Extrinsic( + () => ExtrinsicHelper.api.tx.msa.withdrawTokens(getUnifiedPublicKey(ownerKeys), ownerSignature, payload), + keys + ); + } } diff --git a/e2e/scaffolding/helpers.ts b/e2e/scaffolding/helpers.ts index 4068d87b03..c3f343f1f7 100644 --- a/e2e/scaffolding/helpers.ts +++ b/e2e/scaffolding/helpers.ts @@ -116,7 +116,7 @@ export async function getBlockNumber(): Promise { let cacheED: null | bigint = null; -export async function getExistentialDeposit(): Promise { +export function getExistentialDeposit(): bigint { if (cacheED !== null) return cacheED; return (cacheED = ExtrinsicHelper.api.consts.balances.existentialDeposit.toBigInt()); } diff --git a/pallets/msa/Cargo.toml b/pallets/msa/Cargo.toml index 399764e89d..a55bff8030 100644 --- a/pallets/msa/Cargo.toml +++ b/pallets/msa/Cargo.toml @@ -25,6 +25,7 @@ sp-core = { workspace = true } sp-io = { workspace = true } sp-runtime = { workspace = true } sp-weights = { workspace = true } +pallet-balances = { workspace = true } # Frequency related dependencies common-primitives = { default-features = false, path = "../../common/primitives" } serde = { workspace = true, features = ["derive"] } diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index 5b3cdc6959..d41764b377 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -34,7 +34,13 @@ use frame_support::{ dispatch::{DispatchInfo, DispatchResult, PostDispatchInfo}, pallet_prelude::*, - traits::IsSubType, + traits::{ + tokens::{ + fungible::{Inspect as InspectFungible, Mutate}, + Fortitude, Preservation, + }, + IsSubType, + }, }; use lazy_static::lazy_static; use parity_scale_codec::{Decode, Encode}; @@ -53,6 +59,7 @@ use common_primitives::{ }, node::ProposalProvider, schema::{SchemaId, SchemaValidator}, + signatures::{AccountAddressMapper, EthereumAddressMapper}, }; use frame_system::pallet_prelude::*; use scale_info::TypeInfo; @@ -143,6 +150,9 @@ pub mod pallet { /// The Council proposal provider interface type ProposalProvider: ProposalProvider; + + /// Currency type for managing MSA account balances + type Currency: Mutate + InspectFungible; } const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); @@ -306,7 +316,7 @@ pub mod pallet { /// MsaId values have reached the maximum MsaIdOverflow, - /// Cryptographic signature verification failed for adding a key to MSA + /// Cryptographic signature verification failed MsaOwnershipInvalidSignature, /// Ony the MSA Owner may perform the operation @@ -386,6 +396,15 @@ pub mod pallet { /// Attempted to add a new signature to a corrupt signature registry SignatureRegistryCorrupted, + + /// MSA balance is zero (paid transaction) or insufficient to cover fees (free transaction) + InsufficientBalanceToWithdraw, + + /// MSA balance to withdraw is insufficient to fund the destination account such that it would not be reaped + InsufficientBalanceToFundDestination, + + /// Fund transfer error + UnexpectedTokenTransferError, } impl BlockNumberProvider for Pallet { @@ -909,6 +928,103 @@ pub mod pallet { Ok(()) } + + /// Withdraw all available tokens from the account associated with the MSA, to the account of the caller. + /// + /// The `origin` parameter represents the account from which the function is called and must be the account receiving the tokens. + /// + /// The function requires one signature: `msa_owner_proof`, which serve as proof that the owner of the MSA authorizes the transaction. + /// + /// The necessary information for the withdrawal authorization, the destination account and the MSA ID, are contained in the `authorization_payload` parameter of type [AddKeyData]. + /// It also contains an expiration block number for the proof, ensuring it is valid and must be greater than the current block. + /// + /// # Events + /// * [`pallet_balances::::Transfer`] + /// + /// # Errors + /// + /// * [`Error::NotKeyOwner`] - the transaction signer origin is not the owner of the public key contained in the provided `authorization_payload` + /// * [`Error::MsaOwnershipInvalidSignature`] - `msa_owner_public_key` is not a valid signer of the provided `authorization_payload`. + /// * [`Error::NoKeyExists`] - the public key supplied in `msa_owner_public_key` is not a registered control key of any MSA. + /// * [`Error::NotMsaOwner`] - the MSA ID in `authorization_payload` does not match the MSA ID of the `msa_owner_public_key`. + /// * [`Error::ProofHasExpired`] - the current block is less than the `expired` block number set in `AddKeyData`. + /// * [`Error::ProofNotYetValid`] - the `expired` block number set in `AddKeyData` is greater than the current block number plus mortality_block_limit(). + /// * [`Error::SignatureAlreadySubmitted`] - signature has already been used. + /// * [`Error::InsufficientBalanceToWithdraw`] - the MSA account has not balance to withdraw + /// * [`Error::InsufficientBalanceToFundDestination`] - the transfer would result in an account that would be reaped due to existential balance requirements + /// * [`Error::UnexpectedTokenTransferError`] - the balances pallet threw an error during the attempted transfer + /// + #[pallet::call_index(14)] + #[pallet::weight(T::WeightInfo::withdraw_tokens())] + pub fn withdraw_tokens( + origin: OriginFor, + msa_owner_public_key: T::AccountId, + msa_owner_proof: MultiSignature, + authorization_payload: AddKeyData, + ) -> DispatchResult { + let public_key = ensure_signed(origin)?; + + ensure!(public_key == authorization_payload.new_public_key, Error::::NotKeyOwner); + + ensure!( + Self::verify_signature( + &msa_owner_proof, + &msa_owner_public_key, + authorization_payload.encode() + ), + Error::::MsaOwnershipInvalidSignature + ); + + Self::register_signature(&msa_owner_proof, authorization_payload.expiration)?; + + let msa_id = authorization_payload.msa_id; + + Self::ensure_msa_owner(&msa_owner_public_key, msa_id)?; + + // TODO + // - Get account address for MSA + let msa_address = Self::msa_id_to_eth_address(msa_id); + + // - Convert to AccountId + // let msa_account_id: T::AccountId = ::to_account_id(&msa_address.0); + // log::warn!("MSA account ID: {:?}", msa_account_id); + // let msa_address32: AccountId32 = EthereumAddressMapper::to_bytes32(&msa_address.0); + let mut bytes = &EthereumAddressMapper::to_bytes32(&msa_address.0)[..]; + let msa_account_id = T::AccountId::decode(&mut bytes).unwrap(); + + // - Check that the MSA has a balance to withdraw + let msa_balance = T::Currency::reducible_balance( + &msa_account_id, + Preservation::Expendable, + Fortitude::Polite, + ); + ensure!(msa_balance > Zero::zero(), Error::::InsufficientBalanceToWithdraw); + + // - TODO: Phase 2 (free transaction w/fee paid from proceeds): Check that MSA balance is > fee + + let ed = T::Currency::minimum_balance(); + // Make sure either: + // - Balance to be transferred > existential deposit, OR + // - Destination account balance + ammount to be transferred > existential deposit + let amount_alone_is_insufficient = msa_balance < ed; + let dest_will_be_reaped = if amount_alone_is_insufficient { + let dest_balance = T::Currency::total_balance(&public_key); + (dest_balance + msa_balance) < ed + } else { + amount_alone_is_insufficient + }; + + ensure!(!dest_will_be_reaped, Error::::InsufficientBalanceToFundDestination); + + let result = ::Currency::transfer( + &msa_account_id, + &public_key, + msa_balance, + Preservation::Expendable, + ); + ensure!(result.is_ok(), Error::::UnexpectedTokenTransferError); + Ok(()) + } } } diff --git a/pallets/msa/src/types.rs b/pallets/msa/src/types.rs index 35160645d8..71c43617f4 100644 --- a/pallets/msa/src/types.rs +++ b/pallets/msa/src/types.rs @@ -16,7 +16,9 @@ use scale_info::TypeInfo; /// Dispatch Empty pub const EMPTY_FUNCTION: fn(MessageSourceId) -> DispatchResult = |_| Ok(()); -/// A type definition for the payload of adding an MSA key - `pallet_msa::add_public_key_to_msa` +/// A type definition for the payload for the following operations: +/// - Adding an MSA key - `pallet_msa::add_public_key_to_msa` +/// - Authorizing a token withdrawal to an address associated with a public key - `pallet_msa::withdraw_tokens` #[derive( TypeInfo, RuntimeDebugNoBound, Clone, Decode, DecodeWithMemTracking, Encode, PartialEq, Eq, )] @@ -24,7 +26,7 @@ pub const EMPTY_FUNCTION: fn(MessageSourceId) -> DispatchResult = |_| Ok(()); pub struct AddKeyData { /// Message Source Account identifier pub msa_id: MessageSourceId, - /// The block number at which the signed proof for add_public_key_to_msa expires. + /// The block number at which a signed proof of this payload expires. pub expiration: BlockNumberFor, /// The public key to be added. pub new_public_key: T::AccountId, diff --git a/pallets/msa/src/weights.rs b/pallets/msa/src/weights.rs index db27906009..5bab044776 100644 --- a/pallets/msa/src/weights.rs +++ b/pallets/msa/src/weights.rs @@ -45,6 +45,7 @@ pub trait WeightInfo { fn create_provider_via_governance() -> Weight; fn propose_to_be_provider() -> Weight; fn reindex_offchain() -> Weight; + fn withdraw_tokens() -> Weight; } /// Weights for `pallet_msa` using the Substrate node and recommended hardware. @@ -238,6 +239,13 @@ impl WeightInfo for SubstrateWeight { // Minimum execution time: 4_000_000 picoseconds. Weight::from_parts(8_000_000, 0) } + fn withdraw_tokens() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 4_000_000 picoseconds. + Weight::from_parts(8_000_000, 0) + } } // For backwards compatibility and tests. @@ -430,6 +438,13 @@ impl WeightInfo for () { // Minimum execution time: 4_000_000 picoseconds. Weight::from_parts(8_000_000, 0) } + fn withdraw_tokens() -> Weight { + // Proof Size summary in bytes: + // Measured: `0` + // Estimated: `0` + // Minimum execution time: 4_000_000 picoseconds. + Weight::from_parts(8_000_000, 0) + } } diff --git a/runtime/frequency/src/lib.rs b/runtime/frequency/src/lib.rs index bacf8fb287..269c38da56 100644 --- a/runtime/frequency/src/lib.rs +++ b/runtime/frequency/src/lib.rs @@ -587,6 +587,8 @@ impl pallet_msa::Config for Runtime { EnsureRoot, pallet_collective::EnsureMembers, >; + // The Currency type for managing MSA token balances + type Currency = Balances; } parameter_types! { From 563baed8bded11985fad670fb573bd9f2cf29d6f Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Tue, 13 May 2025 11:17:56 -0400 Subject: [PATCH 02/19] fix: doc lint --- pallets/msa/src/lib.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index d41764b377..79c2a6daf5 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -939,7 +939,7 @@ pub mod pallet { /// It also contains an expiration block number for the proof, ensuring it is valid and must be greater than the current block. /// /// # Events - /// * [`pallet_balances::::Transfer`] + /// * [`pallet_balances::Event::::Transfer`] - Transfer token event /// /// # Errors /// @@ -981,14 +981,10 @@ pub mod pallet { Self::ensure_msa_owner(&msa_owner_public_key, msa_id)?; - // TODO // - Get account address for MSA let msa_address = Self::msa_id_to_eth_address(msa_id); // - Convert to AccountId - // let msa_account_id: T::AccountId = ::to_account_id(&msa_address.0); - // log::warn!("MSA account ID: {:?}", msa_account_id); - // let msa_address32: AccountId32 = EthereumAddressMapper::to_bytes32(&msa_address.0); let mut bytes = &EthereumAddressMapper::to_bytes32(&msa_address.0)[..]; let msa_account_id = T::AccountId::decode(&mut bytes).unwrap(); From a7ac8db13168daf6f6de56f628667348d6d62713 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Tue, 13 May 2025 11:18:19 -0400 Subject: [PATCH 03/19] fix: broken unit tests --- pallets/capacity/src/tests/mock.rs | 1 + .../frequency-tx-payment/src/tests/mock.rs | 1 + pallets/msa/src/tests/mock.rs | 23 +++++++++++++++++-- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/pallets/capacity/src/tests/mock.rs b/pallets/capacity/src/tests/mock.rs index 611e140c41..c7d422c67d 100644 --- a/pallets/capacity/src/tests/mock.rs +++ b/pallets/capacity/src/tests/mock.rs @@ -143,6 +143,7 @@ impl pallet_msa::Config for Test { type CreateProviderViaGovernanceOrigin = EnsureSigned; /// This MUST ALWAYS be MaxSignaturesPerBucket * NumberOfBuckets. type MaxSignaturesStored = ConstU32<8000>; + type Currency = pallet_balances::Pallet; } // not used yet diff --git a/pallets/frequency-tx-payment/src/tests/mock.rs b/pallets/frequency-tx-payment/src/tests/mock.rs index 85f6424fc2..4ca565ea27 100644 --- a/pallets/frequency-tx-payment/src/tests/mock.rs +++ b/pallets/frequency-tx-payment/src/tests/mock.rs @@ -169,6 +169,7 @@ impl pallet_msa::Config for Test { type CreateProviderViaGovernanceOrigin = EnsureSigned; /// This MUST ALWAYS be MaxSignaturesPerBucket * NumberOfBuckets. type MaxSignaturesStored = ConstU32<8000>; + type Currency = pallet_balances::Pallet; } // Needs parameter_types! for the impls below diff --git a/pallets/msa/src/tests/mock.rs b/pallets/msa/src/tests/mock.rs index 87f167abc6..74e67e7f27 100644 --- a/pallets/msa/src/tests/mock.rs +++ b/pallets/msa/src/tests/mock.rs @@ -5,7 +5,7 @@ use common_primitives::{ use common_runtime::constants::DAYS; use frame_support::{ assert_ok, parameter_types, - traits::{ConstU16, ConstU32, EitherOfDiverse, OnFinalize, OnInitialize}, + traits::{ConstU16, ConstU32, ConstU64, EitherOfDiverse, OnFinalize, OnInitialize}, weights::Weight, }; use frame_system::EnsureRoot; @@ -38,6 +38,7 @@ frame_support::construct_runtime!( pub enum Test { System: frame_system::{Pallet, Call, Config, Storage, Event}, + Balances: pallet_balances::{Pallet, Call, Storage, Config, Event}, Msa: pallet_msa::{Pallet, Call, Storage, Event}, Schemas: pallet_schemas::{Pallet, Call, Storage, Event}, Council: pallet_collective::::{Pallet, Call, Config, Storage, Event, Origin}, @@ -97,7 +98,7 @@ impl frame_system::Config for Test { type BlockHashCount = ConstU32<250>; type Version = (); type PalletInfo = PalletInfo; - type AccountData = (); + type AccountData = pallet_balances::AccountData; type OnNewAccount = (); type OnKilledAccount = (); type SystemWeightInfo = (); @@ -112,6 +113,23 @@ impl frame_system::Config for Test { type ExtensionsWeightInfo = (); } +impl pallet_balances::Config for Test { + type MaxReserves = (); + type ReserveIdentifier = [u8; 8]; + type MaxLocks = ConstU32<10>; + type Balance = u64; + type RuntimeEvent = RuntimeEvent; + type DustRemoval = (); + type ExistentialDeposit = ConstU64<1>; + type AccountStore = System; + type WeightInfo = (); + type FreezeIdentifier = RuntimeFreezeReason; + type MaxFreezes = ConstU32<2>; + type RuntimeHoldReason = (); + type RuntimeFreezeReason = (); + type DoneSlashHandler = (); +} + impl pallet_schemas::Config for Test { type RuntimeEvent = RuntimeEvent; type WeightInfo = (); @@ -215,6 +233,7 @@ impl pallet_msa::Config for Test { EnsureRoot, pallet_collective::EnsureMembers, >; + type Currency = pallet_balances::Pallet; } pub fn set_max_signature_stored(max: u32) { From 551c6c5de90495aeedeaebeab9669e2227a896b6 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Tue, 13 May 2025 11:47:20 -0400 Subject: [PATCH 04/19] feat: bump spec version --- runtime/frequency/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/frequency/src/lib.rs b/runtime/frequency/src/lib.rs index 269c38da56..7549dc0ec6 100644 --- a/runtime/frequency/src/lib.rs +++ b/runtime/frequency/src/lib.rs @@ -435,7 +435,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: Cow::Borrowed("frequency"), impl_name: Cow::Borrowed("frequency"), authoring_version: 1, - spec_version: 157, + spec_version: 158, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 1, @@ -449,7 +449,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: Cow::Borrowed("frequency-testnet"), impl_name: Cow::Borrowed("frequency"), authoring_version: 1, - spec_version: 157, + spec_version: 158, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 1, From 6a19e6443d2ae17e36d4edbd2efa493b4a824d2e Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Tue, 13 May 2025 12:59:26 -0400 Subject: [PATCH 05/19] fix: update MSA pallet README --- pallets/msa/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/pallets/msa/README.md b/pallets/msa/README.md index 3361828911..20c3133c1b 100644 --- a/pallets/msa/README.md +++ b/pallets/msa/README.md @@ -49,6 +49,7 @@ The MSA pallet provides for: | `retire_msa`
Remove all keys and mark the MSA as retired | Delegator | Free | [`PublicKeyDeleted`](https://frequency-chain.github.io/frequency/pallet_msa/pallet/enum.Event.html#variant.PublicKeyDeleted), [`MsaRetired`](https://frequency-chain.github.io/frequency/pallet_msa/pallet/enum.Event.html#variant.MsaRetired) | 18 | | `revoke_delegation_by_delegator`
Remove delegation | Delegator | Free | [`DelegationRevoked`](https://frequency-chain.github.io/frequency/pallet_msa/pallet/enum.Event.html#variant.DelegationRevoked) | 1 | | `revoke_delegation_by_provider`
Remove delegation | Provider | Free | [`DelegationRevoked`](https://frequency-chain.github.io/frequency/pallet_msa/pallet/enum.Event.html#variant.DelegationRevoked) | 1 | +| `withdraw_tokens`
Withdraw all tokens from an MSA | Token Account | Tokens | [`Transfer`](https://paritytech.github.io/polkadot-sdk/master/pallet_balances/pallet/enum.Event.html#variant.Transfer) | 158 | See [Rust Docs](https://frequency-chain.github.io/frequency/pallet_msa/pallet/struct.Pallet.html) for more details. From b57132e1a99e253cce265432a3cb9abcb6003d7e Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Tue, 13 May 2025 16:19:03 -0400 Subject: [PATCH 06/19] feat: unit tests --- pallets/msa/src/tests/mod.rs | 1 + pallets/msa/src/tests/msa_token_tests.rs | 223 +++++++++++++++++++++++ 2 files changed, 224 insertions(+) create mode 100644 pallets/msa/src/tests/msa_token_tests.rs diff --git a/pallets/msa/src/tests/mod.rs b/pallets/msa/src/tests/mod.rs index 786cd0482d..5d3d76b092 100644 --- a/pallets/msa/src/tests/mod.rs +++ b/pallets/msa/src/tests/mod.rs @@ -3,6 +3,7 @@ pub mod mock; mod creation_tests; mod delegation_tests; mod governance_tests; +mod msa_token_tests; mod offchain_tests; mod other_tests; mod public_key_tests; diff --git a/pallets/msa/src/tests/msa_token_tests.rs b/pallets/msa/src/tests/msa_token_tests.rs new file mode 100644 index 0000000000..746a9760b7 --- /dev/null +++ b/pallets/msa/src/tests/msa_token_tests.rs @@ -0,0 +1,223 @@ +use frame_support::{ + assert_noop, assert_ok, + traits::{Currency, tokens::{Fortitude, Preservation, fungible::Inspect}}, +}; + +use sp_core::{sr25519, Encode, Pair}; +use sp_runtime::{MultiSignature}; + +use crate::{ + tests::mock::*, + types::AddKeyData, + Config, Error, +}; + +use common_primitives::{ + msa::{H160, MessageSourceId}, + node::BlockNumber, + signatures::{AccountAddressMapper, EthereumAddressMapper}, + utils::wrap_binary_data, +}; + +use pallet_balances::Event as BalancesEvent; + +fn generate_payload(msa_id: MessageSourceId, msa_owner_keys: &sr25519::Pair, new_public_key: &sr25519::Pair, expiration: Option) -> (AddKeyData::, Vec, MultiSignature) { + let payload = AddKeyData:: { + msa_id, + expiration: match expiration { + Some(block_number) => block_number, + None => 10 + }, + new_public_key: new_public_key.public().into() + }; + + let encoded_payload = wrap_binary_data(payload.encode()); + let signature: MultiSignature = msa_owner_keys.sign(&encoded_payload).into(); + + (payload, encoded_payload, signature) +} + +#[test] +fn it_fails_when_caller_key_does_not_match_payload() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + let (other_key_pair, _) = sr25519::Pair::generate(); + + let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &other_key_pair, None); + + assert_noop!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature, + payload + ), + Error::::NotKeyOwner + ); + }); +} + +#[test] +fn it_fails_when_payload_signature_is_invalid() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + let (other_key_pair, _) = sr25519::Pair::generate(); + + let (payload, _, msa_signature) = generate_payload(msa_id, &other_key_pair, &origin_key_pair, None); + + assert_noop!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature, + payload + ), + Error::::MsaOwnershipInvalidSignature + ); + }); +} + +#[test] +fn it_fails_when_proof_is_expired() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + + // The current block is 1, therefore setting the proof expiration to 1 should cause + // the extrinsic to fail because the proof has expired. + let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, Some(1)); + + assert_noop!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature, + payload + ), + Error::::ProofHasExpired + ); + }); +} + +#[test] +fn it_fails_when_proof_is_not_yet_valid() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + + // The current block is 1, therefore setting the proof expiration to the max mortality period + // should cause the extrinsic to fail + let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, Some(Msa::mortality_block_limit(1))); + + assert_noop!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature, + payload + ), + Error::::ProofNotYetValid + ); + }); +} + +#[test] +fn it_fails_when_msa_key_is_not_an_msa_control_key() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + + let (payload, _, msa_signature) = generate_payload(msa_id + 1, &owner_key_pair, &origin_key_pair, None); + + assert_noop!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature, + payload + ), + Error::::NotMsaOwner + ); + }) +} + +#[test] +fn it_fails_when_msa_key_does_not_control_msa_in_payload() { + new_test_ext().execute_with(|| { + let (msa_id, _) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + let (other_key_pair, _) = sr25519::Pair::generate(); + + let (payload, _, msa_signature) = generate_payload(msa_id, &other_key_pair, &origin_key_pair, None); + + assert_noop!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + other_key_pair.public().into(), + msa_signature, + payload + ), + Error::::NoKeyExists + ); + }) +} + +#[test] +fn it_fails_when_msa_does_not_have_a_balance() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + + let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); + + assert_noop!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature, + payload + ), + Error::::InsufficientBalanceToWithdraw + ); + }) +} + +#[test] +fn it_succeeds_when_balance_is_sufficient() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + let eth_account_id: H160 = Msa::msa_id_to_eth_address(msa_id); + let bytes = EthereumAddressMapper::to_bytes32(ð_account_id.0); + let msa_account_id = ::AccountId::from(bytes.clone()); + + let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); + + let transfer_amount = 10_000_000; + + // Fund MSA + let _ = ::Currency::deposit_creating(&msa_account_id, transfer_amount); + + assert_ok!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature, + payload + ) + ); + + let receiver_balance = ::Currency::reducible_balance( + &origin_key_pair.public().into(), + Preservation::Expendable, + Fortitude::Polite + ); + assert_eq!(receiver_balance, transfer_amount, "transfer amount {} does not equal new balance {}", transfer_amount, receiver_balance); + + System::assert_last_event( + RuntimeEvent::Balances(BalancesEvent::Transfer { from: msa_account_id, to: origin_key_pair.public().into(), amount: transfer_amount }), + ); + }) +} From 1e782445f15ffaa021e7db5ba5a0d74b91a31f2a Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Tue, 13 May 2025 17:25:40 -0400 Subject: [PATCH 07/19] feat: add benchmarks for withdraw_tokens extrinsic --- pallets/msa/src/benchmarking.rs | 40 +++++++++++++++++++++++- pallets/msa/src/tests/msa_token_tests.rs | 4 +-- pallets/msa/src/weights.rs | 30 +++++++++++++++--- 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/pallets/msa/src/benchmarking.rs b/pallets/msa/src/benchmarking.rs index 2ccc3e05cd..d553419752 100644 --- a/pallets/msa/src/benchmarking.rs +++ b/pallets/msa/src/benchmarking.rs @@ -5,7 +5,7 @@ use crate::types::EMPTY_FUNCTION; #[allow(unused)] use crate::Pallet as Msa; use frame_benchmarking::{account, v2::*}; -use frame_support::assert_ok; +use frame_support::{assert_ok, traits::fungible::Inspect}; use frame_system::RawOrigin; use sp_core::{crypto::KeyTypeId, Encode}; use sp_runtime::RuntimeAppPublic; @@ -369,6 +369,44 @@ mod benchmarks { Ok(()) } + #[benchmark] + fn withdraw_tokens() -> Result<(), BenchmarkError> { + prep_signature_registry::(); + + let (msa_public_key, msa_key_pair, msa_id) = + create_msa_account_and_keys::(); + + let eth_account_id: H160 = Msa::::msa_id_to_eth_address(msa_id); + let mut bytes = &EthereumAddressMapper::to_bytes32(ð_account_id.0)[..]; + let msa_account_id = ::AccountId::decode(&mut bytes).unwrap(); + + // Fund MSA + // let balance = <::Currency as Inspect<::AccountId>>::Balance.from(10_000_000u128); + let balance = ::Currency::minimum_balance(); + T::Currency::set_balance(&msa_account_id, balance); + assert_eq!(T::Currency::balance(&msa_account_id), balance); + + let (add_key_payload, _, new_account_id) = + add_key_payload_and_signature::(msa_id); + + let encoded_add_key_payload = wrap_binary_data(add_key_payload.encode()); + let owner_signature = MultiSignature::Sr25519( + msa_key_pair.sign(&encoded_add_key_payload).unwrap().into(), + ); + + #[extrinsic_call] + _( + RawOrigin::Signed(new_account_id.clone()), + msa_public_key.clone(), + owner_signature, + add_key_payload, + ); + + assert_eq!(T::Currency::balance(&msa_account_id), Zero::zero()); + Ok(()) + } + + impl_benchmark_test_suite!( Msa, crate::tests::mock::new_test_ext_keystore(), diff --git a/pallets/msa/src/tests/msa_token_tests.rs b/pallets/msa/src/tests/msa_token_tests.rs index 746a9760b7..3c9d41bb60 100644 --- a/pallets/msa/src/tests/msa_token_tests.rs +++ b/pallets/msa/src/tests/msa_token_tests.rs @@ -190,8 +190,8 @@ fn it_succeeds_when_balance_is_sufficient() { let (msa_id, owner_key_pair) = create_account(); let (origin_key_pair, _) = sr25519::Pair::generate(); let eth_account_id: H160 = Msa::msa_id_to_eth_address(msa_id); - let bytes = EthereumAddressMapper::to_bytes32(ð_account_id.0); - let msa_account_id = ::AccountId::from(bytes.clone()); + let bytes: [u8; 32] = EthereumAddressMapper::to_bytes32(ð_account_id.0); + let msa_account_id = ::AccountId::from(bytes); let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); diff --git a/pallets/msa/src/weights.rs b/pallets/msa/src/weights.rs index 5bab044776..f256abc2b4 100644 --- a/pallets/msa/src/weights.rs +++ b/pallets/msa/src/weights.rs @@ -438,12 +438,22 @@ impl WeightInfo for () { // Minimum execution time: 4_000_000 picoseconds. Weight::from_parts(8_000_000, 0) } + /// Storage: `Msa::PayloadSignatureRegistryList` (r:2 w:2) + /// Proof: `Msa::PayloadSignatureRegistryList` (`max_values`: Some(50000), `max_size`: Some(144), added: 2124, mode: `MaxEncodedLen`) + /// Storage: `Msa::PayloadSignatureRegistryPointer` (r:1 w:1) + /// Proof: `Msa::PayloadSignatureRegistryPointer` (`max_values`: Some(1), `max_size`: Some(140), added: 635, mode: `MaxEncodedLen`) + /// Storage: `Msa::PublicKeyToMsaId` (r:1 w:0) + /// Proof: `Msa::PublicKeyToMsaId` (`max_values`: None, `max_size`: Some(48), added: 2523, mode: `MaxEncodedLen`) + /// Storage: `System::Account` (r:2 w:2) + /// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`) fn withdraw_tokens() -> Weight { // Proof Size summary in bytes: - // Measured: `0` - // Estimated: `0` - // Minimum execution time: 4_000_000 picoseconds. - Weight::from_parts(8_000_000, 0) + // Measured: `1270` + // Estimated: `6691` + // Minimum execution time: 151_000_000 picoseconds. + Weight::from_parts(162_000_000, 6691) + .saturating_add(RocksDbWeight::get().reads(6_u64)) + .saturating_add(RocksDbWeight::get().writes(5_u64)) } } @@ -602,4 +612,16 @@ mod tests { > 4107 ); } + #[test] + fn test_withdraw_tokens() { + assert!( + BlockWeights::get() + .per_class + .get(frame_support::dispatch::DispatchClass::Normal) + .max_extrinsic + .unwrap_or_else(::max_value) + .proof_size() + > 6691 + ); + } } From 03634577b31cd334dba8338ff859401eaf42dcea Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Tue, 13 May 2025 17:53:46 -0400 Subject: [PATCH 08/19] fix: fix payload signature verification after merge EIP-712 from main --- pallets/msa/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index ce19636153..6bdaf4c1de 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -966,7 +966,7 @@ pub mod pallet { Self::verify_signature( &msa_owner_proof, &msa_owner_public_key, - authorization_payload.encode() + &authorization_payload ), Error::::MsaOwnershipInvalidSignature ); From 1c7328fc69049b401a9216ce3d4f33f1ac90bffc Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Wed, 14 May 2025 09:43:45 -0400 Subject: [PATCH 09/19] fix: bump spec version --- runtime/frequency/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/frequency/src/lib.rs b/runtime/frequency/src/lib.rs index 7549dc0ec6..6fc2a67b03 100644 --- a/runtime/frequency/src/lib.rs +++ b/runtime/frequency/src/lib.rs @@ -435,7 +435,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: Cow::Borrowed("frequency"), impl_name: Cow::Borrowed("frequency"), authoring_version: 1, - spec_version: 158, + spec_version: 159, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 1, @@ -449,7 +449,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: Cow::Borrowed("frequency-testnet"), impl_name: Cow::Borrowed("frequency"), authoring_version: 1, - spec_version: 158, + spec_version: 159, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 1, From fa04839b3a48a4034fc9b2b6cbb1938cc63f63a3 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Wed, 14 May 2025 09:44:43 -0400 Subject: [PATCH 10/19] fix: formatting --- pallets/msa/src/benchmarking.rs | 12 ++-- pallets/msa/src/tests/msa_token_tests.rs | 84 +++++++++++++++--------- 2 files changed, 56 insertions(+), 40 deletions(-) diff --git a/pallets/msa/src/benchmarking.rs b/pallets/msa/src/benchmarking.rs index d553419752..6642ab7e04 100644 --- a/pallets/msa/src/benchmarking.rs +++ b/pallets/msa/src/benchmarking.rs @@ -373,8 +373,7 @@ mod benchmarks { fn withdraw_tokens() -> Result<(), BenchmarkError> { prep_signature_registry::(); - let (msa_public_key, msa_key_pair, msa_id) = - create_msa_account_and_keys::(); + let (msa_public_key, msa_key_pair, msa_id) = create_msa_account_and_keys::(); let eth_account_id: H160 = Msa::::msa_id_to_eth_address(msa_id); let mut bytes = &EthereumAddressMapper::to_bytes32(ð_account_id.0)[..]; @@ -386,13 +385,11 @@ mod benchmarks { T::Currency::set_balance(&msa_account_id, balance); assert_eq!(T::Currency::balance(&msa_account_id), balance); - let (add_key_payload, _, new_account_id) = - add_key_payload_and_signature::(msa_id); + let (add_key_payload, _, new_account_id) = add_key_payload_and_signature::(msa_id); let encoded_add_key_payload = wrap_binary_data(add_key_payload.encode()); - let owner_signature = MultiSignature::Sr25519( - msa_key_pair.sign(&encoded_add_key_payload).unwrap().into(), - ); + let owner_signature = + MultiSignature::Sr25519(msa_key_pair.sign(&encoded_add_key_payload).unwrap().into()); #[extrinsic_call] _( @@ -406,7 +403,6 @@ mod benchmarks { Ok(()) } - impl_benchmark_test_suite!( Msa, crate::tests::mock::new_test_ext_keystore(), diff --git a/pallets/msa/src/tests/msa_token_tests.rs b/pallets/msa/src/tests/msa_token_tests.rs index 3c9d41bb60..21b4c1f36e 100644 --- a/pallets/msa/src/tests/msa_token_tests.rs +++ b/pallets/msa/src/tests/msa_token_tests.rs @@ -1,19 +1,18 @@ use frame_support::{ assert_noop, assert_ok, - traits::{Currency, tokens::{Fortitude, Preservation, fungible::Inspect}}, + traits::{ + tokens::{fungible::Inspect, Fortitude, Preservation}, + Currency, + }, }; use sp_core::{sr25519, Encode, Pair}; -use sp_runtime::{MultiSignature}; +use sp_runtime::MultiSignature; -use crate::{ - tests::mock::*, - types::AddKeyData, - Config, Error, -}; +use crate::{tests::mock::*, types::AddKeyData, Config, Error}; use common_primitives::{ - msa::{H160, MessageSourceId}, + msa::{MessageSourceId, H160}, node::BlockNumber, signatures::{AccountAddressMapper, EthereumAddressMapper}, utils::wrap_binary_data, @@ -21,14 +20,19 @@ use common_primitives::{ use pallet_balances::Event as BalancesEvent; -fn generate_payload(msa_id: MessageSourceId, msa_owner_keys: &sr25519::Pair, new_public_key: &sr25519::Pair, expiration: Option) -> (AddKeyData::, Vec, MultiSignature) { +fn generate_payload( + msa_id: MessageSourceId, + msa_owner_keys: &sr25519::Pair, + new_public_key: &sr25519::Pair, + expiration: Option, +) -> (AddKeyData, Vec, MultiSignature) { let payload = AddKeyData:: { msa_id, expiration: match expiration { Some(block_number) => block_number, - None => 10 + None => 10, }, - new_public_key: new_public_key.public().into() + new_public_key: new_public_key.public().into(), }; let encoded_payload = wrap_binary_data(payload.encode()); @@ -44,7 +48,8 @@ fn it_fails_when_caller_key_does_not_match_payload() { let (origin_key_pair, _) = sr25519::Pair::generate(); let (other_key_pair, _) = sr25519::Pair::generate(); - let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &other_key_pair, None); + let (payload, _, msa_signature) = + generate_payload(msa_id, &owner_key_pair, &other_key_pair, None); assert_noop!( Msa::withdraw_tokens( @@ -65,7 +70,8 @@ fn it_fails_when_payload_signature_is_invalid() { let (origin_key_pair, _) = sr25519::Pair::generate(); let (other_key_pair, _) = sr25519::Pair::generate(); - let (payload, _, msa_signature) = generate_payload(msa_id, &other_key_pair, &origin_key_pair, None); + let (payload, _, msa_signature) = + generate_payload(msa_id, &other_key_pair, &origin_key_pair, None); assert_noop!( Msa::withdraw_tokens( @@ -87,7 +93,8 @@ fn it_fails_when_proof_is_expired() { // The current block is 1, therefore setting the proof expiration to 1 should cause // the extrinsic to fail because the proof has expired. - let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, Some(1)); + let (payload, _, msa_signature) = + generate_payload(msa_id, &owner_key_pair, &origin_key_pair, Some(1)); assert_noop!( Msa::withdraw_tokens( @@ -109,7 +116,12 @@ fn it_fails_when_proof_is_not_yet_valid() { // The current block is 1, therefore setting the proof expiration to the max mortality period // should cause the extrinsic to fail - let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, Some(Msa::mortality_block_limit(1))); + let (payload, _, msa_signature) = generate_payload( + msa_id, + &owner_key_pair, + &origin_key_pair, + Some(Msa::mortality_block_limit(1)), + ); assert_noop!( Msa::withdraw_tokens( @@ -129,7 +141,8 @@ fn it_fails_when_msa_key_is_not_an_msa_control_key() { let (msa_id, owner_key_pair) = create_account(); let (origin_key_pair, _) = sr25519::Pair::generate(); - let (payload, _, msa_signature) = generate_payload(msa_id + 1, &owner_key_pair, &origin_key_pair, None); + let (payload, _, msa_signature) = + generate_payload(msa_id + 1, &owner_key_pair, &origin_key_pair, None); assert_noop!( Msa::withdraw_tokens( @@ -150,7 +163,8 @@ fn it_fails_when_msa_key_does_not_control_msa_in_payload() { let (origin_key_pair, _) = sr25519::Pair::generate(); let (other_key_pair, _) = sr25519::Pair::generate(); - let (payload, _, msa_signature) = generate_payload(msa_id, &other_key_pair, &origin_key_pair, None); + let (payload, _, msa_signature) = + generate_payload(msa_id, &other_key_pair, &origin_key_pair, None); assert_noop!( Msa::withdraw_tokens( @@ -170,7 +184,8 @@ fn it_fails_when_msa_does_not_have_a_balance() { let (msa_id, owner_key_pair) = create_account(); let (origin_key_pair, _) = sr25519::Pair::generate(); - let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); + let (payload, _, msa_signature) = + generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); assert_noop!( Msa::withdraw_tokens( @@ -193,31 +208,36 @@ fn it_succeeds_when_balance_is_sufficient() { let bytes: [u8; 32] = EthereumAddressMapper::to_bytes32(ð_account_id.0); let msa_account_id = ::AccountId::from(bytes); - let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); + let (payload, _, msa_signature) = + generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); let transfer_amount = 10_000_000; // Fund MSA let _ = ::Currency::deposit_creating(&msa_account_id, transfer_amount); - assert_ok!( - Msa::withdraw_tokens( - RuntimeOrigin::signed(origin_key_pair.public().into()), - owner_key_pair.public().into(), - msa_signature, - payload - ) - ); + assert_ok!(Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature, + payload + )); let receiver_balance = ::Currency::reducible_balance( &origin_key_pair.public().into(), Preservation::Expendable, - Fortitude::Polite + Fortitude::Polite, ); - assert_eq!(receiver_balance, transfer_amount, "transfer amount {} does not equal new balance {}", transfer_amount, receiver_balance); - - System::assert_last_event( - RuntimeEvent::Balances(BalancesEvent::Transfer { from: msa_account_id, to: origin_key_pair.public().into(), amount: transfer_amount }), + assert_eq!( + receiver_balance, transfer_amount, + "transfer amount {} does not equal new balance {}", + transfer_amount, receiver_balance ); + + System::assert_last_event(RuntimeEvent::Balances(BalancesEvent::Transfer { + from: msa_account_id, + to: origin_key_pair.public().into(), + amount: transfer_amount, + })); }) } From 1528d195e3efb59351004e0e4257d3cbc72c20b6 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Wed, 21 May 2025 13:50:28 -0400 Subject: [PATCH 11/19] feat: free extrinsic - address some PR comments - make extrinsic free - update tests for free extrinsic --- e2e/msa/msaTokens.test.ts | 160 +++++++++++-------- e2e/scaffolding/extrinsicHelpers.ts | 25 ++- e2e/scaffolding/helpers.ts | 13 ++ pallets/msa/Cargo.toml | 2 +- pallets/msa/src/benchmarking.rs | 26 ++- pallets/msa/src/lib.rs | 191 ++++++++++++++++++----- pallets/msa/src/tests/msa_token_tests.rs | 126 ++++++++------- pallets/msa/src/tests/other_tests.rs | 35 ++++- pallets/msa/src/types.rs | 52 +++++- tools/eth-migration/metamask.html | 46 ++++++ 10 files changed, 505 insertions(+), 171 deletions(-) diff --git a/e2e/msa/msaTokens.test.ts b/e2e/msa/msaTokens.test.ts index 84eb0a7d08..116e6aac6a 100644 --- a/e2e/msa/msaTokens.test.ts +++ b/e2e/msa/msaTokens.test.ts @@ -1,6 +1,6 @@ import '@frequency-chain/api-augment'; import assert from 'assert'; -import { AddKeyData, ExtrinsicHelper } from '../scaffolding/extrinsicHelpers'; +import { AuthorizedKeyData, ExtrinsicHelper } from '../scaffolding/extrinsicHelpers'; import { ethereumAddressToKeyringPair, getUnifiedAddress, getUnifiedPublicKey } from '../scaffolding/ethereum'; import { getFundingSource } from '../scaffolding/funding'; import { H160 } from '@polkadot/types/interfaces'; @@ -12,7 +12,7 @@ import { createAndFundKeypair, createKeys, DOLLARS, - generateAddKeyPayload, + generateAuthorizedKeyPayload, getExistentialDeposit, signPayloadSr25519, Sr25519Signature, @@ -115,127 +115,163 @@ describe('MSAs Holding Tokens', function () { }); describe('withdrawTokens', function () { - let keys: KeyringPair; + let msaKeys: KeyringPair; let msaId: u64; let msaAddress: H160; + let otherMsaKeys: KeyringPair; let secondaryKey: KeyringPair; - const defaultPayload: AddKeyData = {}; - let payload: AddKeyData; + let defaultPayload: AuthorizedKeyData; + let payload: AuthorizedKeyData; let ownerSig: Sr25519Signature; let badSig: Sr25519Signature; - let addKeyData: Codec; + let authorizedKeyData: Codec; before(async function () { // Setup an MSA with tokens - keys = await createAndFundKeypair(fundingSource, 5n * CENTS); - const { target } = await ExtrinsicHelper.createMsa(keys).signAndSend(); + msaKeys = await createAndFundKeypair(fundingSource, 5n * CENTS); + let { target } = await ExtrinsicHelper.createMsa(msaKeys).signAndSend(); assert.notEqual(target?.data.msaId, undefined, 'MSA Id not in expected event'); msaId = target!.data.msaId; + // Setup another MSA control key + otherMsaKeys = await createAndFundKeypair(fundingSource, 5n * CENTS); + ({ target } = await ExtrinsicHelper.createMsa(otherMsaKeys).signAndSend()); + assert.notEqual(target?.data.msaId, undefined, 'MSA Id not in expected event'); + const { accountId } = await ExtrinsicHelper.apiPromise.call.msaRuntimeApi.getEthereumAddressForMsaId(msaId); msaAddress = accountId; - secondaryKey = await createAndFundKeypair(fundingSource, 5n * CENTS); + // Create unfunded keys; this extrinsic should be free + secondaryKey = createKeys(); // Default payload making it easier to test `withdrawTokens` - defaultPayload.msaId = msaId; - defaultPayload.newPublicKey = getUnifiedPublicKey(secondaryKey); + defaultPayload = { + msaId, + authorizedPublicKey: getUnifiedPublicKey(secondaryKey), + }; }); beforeEach(async function () { - payload = await generateAddKeyPayload(defaultPayload); + payload = await generateAuthorizedKeyPayload(defaultPayload); }); it('should fail if origin is not address contained in the payload (NotKeyOwner)', async function () { - const badPayload = { ...payload, newPublicKey: getUnifiedAddress(createKeys()) }; // Invalid MSA ID - addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', badPayload); - ownerSig = signPayloadSr25519(keys, addKeyData); - const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, badPayload); - await assert.rejects(op.fundAndSend(fundingSource), { - name: 'NotKeyOwner', + const badPayload = { ...payload, authorizedPublicKey: getUnifiedAddress(createKeys()) }; // Invalid MSA ID + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', badPayload); + ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, badPayload); + await assert.rejects(op.signAndSend(), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 5', // NotKeyOwner, }); }); it('should fail if MSA owner signature is invalid (MsaOwnershipInvalidSignature)', async function () { - addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', payload); - badSig = signPayloadSr25519(createKeys(), addKeyData); // Invalid signature - const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, badSig, payload); - await assert.rejects(op.fundAndSend(fundingSource), { - name: 'MsaOwnershipInvalidSignature', + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); + badSig = signPayloadSr25519(createKeys(), authorizedKeyData); // Invalid signature + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, badSig, payload); + await assert.rejects(op.signAndSend(), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 8', // MsaOwnershipInvalidSignature }); }); it('should fail if expiration has passed (ProofHasExpired)', async function () { - const newPayload = await generateAddKeyPayload({ + const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber(), }); - addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); - ownerSig = signPayloadSr25519(keys, addKeyData); - const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, newPayload); - await assert.rejects(op.fundAndSend(fundingSource), { - name: 'ProofHasExpired', + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); + ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); + await assert.rejects(op.signAndSend(), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 14', // ProofHasExpired, }); }); it('should fail if expiration is not yet valid (ProofNotYetValid)', async function () { const maxMortality = ExtrinsicHelper.api.consts.msa.mortalityWindowSize.toNumber(); - const newPayload = await generateAddKeyPayload({ + const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber() + maxMortality + 999, }); - addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); - ownerSig = signPayloadSr25519(keys, addKeyData); - const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, newPayload); - await assert.rejects(op.fundAndSend(fundingSource), { - name: 'ProofNotYetValid', + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); + ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); + await assert.rejects(op.signAndSend(), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 13', // ProofNotYetValid, + }); + }); + + it('should fail if origin is an MSA control key (IneligibleOrigin)', async function () { + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); + const newPayload = await generateAuthorizedKeyPayload({ + ...defaultPayload, + authorizedPublicKey: getUnifiedPublicKey(otherMsaKeys), + }); + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); + ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + const op = ExtrinsicHelper.withdrawTokens(otherMsaKeys, msaKeys, ownerSig, newPayload); + await assert.rejects(op.signAndSend(), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 12', // IneligibleOrigin, }); }); it('should fail if payload signer does not control the MSA in the signed payload (NotMsaOwner)', async function () { - const newPayload = await generateAddKeyPayload({ + const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, msaId: new u64(ExtrinsicHelper.api.registry, 9999), }); - addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); - ownerSig = signPayloadSr25519(keys, addKeyData); - const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, newPayload); - await assert.rejects(op.fundAndSend(fundingSource), { - name: 'NotMsaOwner', + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); + ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); + await assert.rejects(op.signAndSend(), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 17', // NotMsaOwner, }); }); it('should fail if payload signer is not an MSA control key (NoKeyExists)', async function () { const badSigner = createKeys(); - const newPayload = await generateAddKeyPayload({ + const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, msaId: new u64(ExtrinsicHelper.api.registry, 9999), }); - addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); - ownerSig = signPayloadSr25519(badSigner, addKeyData); + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); + ownerSig = signPayloadSr25519(badSigner, authorizedKeyData); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, badSigner, ownerSig, newPayload); - await assert.rejects(op.fundAndSend(fundingSource), { - name: 'NoKeyExists', + await assert.rejects(op.signAndSend(), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 16', // NoKeyExists, }); }); it('should fail if MSA does not have a balance', async function () { - addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', payload); - ownerSig = signPayloadSr25519(keys, addKeyData); - const op = ExtrinsicHelper.withdrawTokens(secondaryKey, keys, ownerSig, payload); - await assert.rejects(op.fundAndSend(fundingSource), { - name: 'InsufficientBalanceToWithdraw', + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); + ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); + await assert.rejects(op.signAndSend(), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 9', // InsufficientBalanceToWithdraw, }); }); it('should succeed', async function () { - // Fund receiver with known amount to pay for transaction - const startingAmount = 1n * DOLLARS; const transferAmount = 1n * DOLLARS; - const tertiaryKeys = await createAndFundKeypair(fundingSource, startingAmount); const { data: { free: startingBalance }, - } = await ExtrinsicHelper.getAccountInfo(tertiaryKeys); + } = await ExtrinsicHelper.getAccountInfo(secondaryKey); // Send tokens to MSA try { @@ -249,20 +285,18 @@ describe('MSAs Holding Tokens', function () { console.error('Error sending tokens to MSA', err.message); } - const newPayload = await generateAddKeyPayload({ ...payload, newPublicKey: getUnifiedPublicKey(tertiaryKeys) }); - addKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAddKeyData', newPayload); - ownerSig = signPayloadSr25519(keys, addKeyData); - const op = ExtrinsicHelper.withdrawTokens(tertiaryKeys, keys, ownerSig, newPayload); - const { eventMap } = await op.fundAndSend(fundingSource); - const feeAmount = (eventMap['transactionPayment.TransactionFeePaid'].data as unknown as any).actualFee; + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); + ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); + await assert.doesNotReject(op.signAndSend(), 'token transfer transaction should have succeeded'); // Destination account should have had balance increased const { data: { free: endingBalance }, - } = await ExtrinsicHelper.getAccountInfo(tertiaryKeys); + } = await ExtrinsicHelper.getAccountInfo(secondaryKey); assert( - startingBalance.toBigInt() + transferAmount - feeAmount.toBigInt() === endingBalance.toBigInt(), + startingBalance.toBigInt() + transferAmount === endingBalance.toBigInt(), 'balance of recieve should have increased by the transfer amount minus fee' ); }); diff --git a/e2e/scaffolding/extrinsicHelpers.ts b/e2e/scaffolding/extrinsicHelpers.ts index a9652c9dd8..7b93691fe8 100644 --- a/e2e/scaffolding/extrinsicHelpers.ts +++ b/e2e/scaffolding/extrinsicHelpers.ts @@ -4,12 +4,7 @@ import { ApiPromise, ApiRx } from '@polkadot/api'; import { ApiTypes, AugmentedEvent, SubmittableExtrinsic, SignerOptions } from '@polkadot/api/types'; import { KeyringPair } from '@polkadot/keyring/types'; import { Compact, u128, u16, u32, u64, Vec, Option, Bool } from '@polkadot/types'; -import { - FrameSystemAccountInfo, - PalletTimeReleaseReleaseSchedule, - SpRuntimeDispatchError, - PalletSchedulerScheduled, -} from '@polkadot/types/lookup'; +import { FrameSystemAccountInfo, SpRuntimeDispatchError } from '@polkadot/types/lookup'; import { AnyJson, AnyNumber, AnyTuple, Codec, IEvent, ISubmittableResult } from '@polkadot/types/types'; import { firstValueFrom, filter, map, pipe, tap } from 'rxjs'; import { getBlockNumber, getExistentialDeposit, getFinalizedBlockNumber, log, MultiSignatureType } from './helpers'; @@ -31,6 +26,7 @@ import { u8aWrapBytes } from '@polkadot/util'; import type { AccountId32, Call, H256 } from '@polkadot/types/interfaces/runtime'; import { hasRelayChain } from './env'; import { getUnifiedAddress, getUnifiedPublicKey } from './ethereum'; +import { RpcErrorInterface } from '@polkadot/rpc-provider/types'; export interface ReleaseSchedule { start: number; @@ -44,6 +40,11 @@ export interface AddKeyData { expiration?: any; newPublicKey?: any; } +export interface AuthorizedKeyData { + msaId: u64; + expiration?: AnyNumber; + authorizedPublicKey: KeyringPair['publicKey']; +} export interface AddProviderPayload { authorizedMsaId?: u64; schemaIds?: u16[]; @@ -91,6 +92,10 @@ export interface PaginatedDeleteSignaturePayloadV2 { expiration?: any; } +export function isRpcError(e: any): e is RpcErrorInterface { + return e?.name === 'RpcError'; +} + export class EventError extends Error { name: string = ''; message: string = ''; @@ -188,6 +193,7 @@ export class Extrinsic { @@ -205,8 +211,11 @@ export class Extrinsic { + const { expiration, ...payload } = payloadInputs; + return { + expiration: expiration || (blockNumber || (await getBlockNumber())) + expirationOffset, + ...payload, + }; +} + export async function generateItemizedSignaturePayload( payloadInputs: ItemizedSignaturePayloadV2, expirationOffset: number = 100, diff --git a/pallets/msa/Cargo.toml b/pallets/msa/Cargo.toml index a55bff8030..1dbd8e2ca7 100644 --- a/pallets/msa/Cargo.toml +++ b/pallets/msa/Cargo.toml @@ -25,7 +25,6 @@ sp-core = { workspace = true } sp-io = { workspace = true } sp-runtime = { workspace = true } sp-weights = { workspace = true } -pallet-balances = { workspace = true } # Frequency related dependencies common-primitives = { default-features = false, path = "../../common/primitives" } serde = { workspace = true, features = ["derive"] } @@ -34,6 +33,7 @@ hex = { workspace = true, default-features = false, features = ["alloc"] } common-runtime = { path = "../../runtime/common", default-features = false } [dev-dependencies] +pallet-balances = { workspace = true } pallet-schemas = { path = "../schemas", default-features = false } pallet-handles = { path = "../handles", default-features = false } pallet-collective = { workspace = true } diff --git a/pallets/msa/src/benchmarking.rs b/pallets/msa/src/benchmarking.rs index 6642ab7e04..f2aa6ff38d 100644 --- a/pallets/msa/src/benchmarking.rs +++ b/pallets/msa/src/benchmarking.rs @@ -56,6 +56,25 @@ fn add_key_payload_and_signature( (add_key_payload, MultiSignature::Sr25519(signature.into()), acc) } +fn withdraw_tokens_payload_and_signature( + msa_id: u64, + msa_key_pair: SignerId, +) -> (AuthorizedKeyData, MultiSignature, T::AccountId) { + let new_keys = SignerId::generate_pair(None); + let public_key = T::AccountId::decode(&mut &new_keys.encode()[..]).unwrap(); + let withdraw_tokens_payload = AuthorizedKeyData:: { + msa_id, + expiration: 10u32.into(), + authorized_public_key: public_key, + }; + + let encoded_withdraw_tokens_payload = wrap_binary_data(withdraw_tokens_payload.encode()); + + let signature = msa_key_pair.sign(&encoded_withdraw_tokens_payload).unwrap(); + let acc = T::AccountId::decode(&mut &new_keys.encode()[..]).unwrap(); + (withdraw_tokens_payload, MultiSignature::Sr25519(signature.into()), acc) +} + fn create_msa_account_and_keys() -> (T::AccountId, SignerId, MessageSourceId) { let key_pair = SignerId::generate_pair(None); let account_id = T::AccountId::decode(&mut &key_pair.encode()[..]).unwrap(); @@ -385,11 +404,8 @@ mod benchmarks { T::Currency::set_balance(&msa_account_id, balance); assert_eq!(T::Currency::balance(&msa_account_id), balance); - let (add_key_payload, _, new_account_id) = add_key_payload_and_signature::(msa_id); - - let encoded_add_key_payload = wrap_binary_data(add_key_payload.encode()); - let owner_signature = - MultiSignature::Sr25519(msa_key_pair.sign(&encoded_add_key_payload).unwrap().into()); + let (add_key_payload, owner_signature, new_account_id) = + withdraw_tokens_payload_and_signature::(msa_id, msa_key_pair); #[extrinsic_call] _( diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index 6bdaf4c1de..1a0f67e4a0 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -82,7 +82,9 @@ pub use common_primitives::{ handles::HandleProvider, msa::MessageSourceId, node::EIP712Encode, utils::wrap_binary_data, }; pub use pallet::*; -pub use types::{AddKeyData, AddProvider, PermittedDelegationSchemas, EMPTY_FUNCTION}; +pub use types::{ + AddKeyData, AddProvider, AuthorizedKeyData, PermittedDelegationSchemas, EMPTY_FUNCTION, +}; pub use weights::*; /// Offchain storage for MSA pallet @@ -405,6 +407,9 @@ pub mod pallet { /// Fund transfer error UnexpectedTokenTransferError, + + /// Origin is ineleligible for the current transaction + IneligibleOrigin, } impl BlockNumberProvider for Pallet { @@ -951,32 +956,17 @@ pub mod pallet { /// * [`Error::UnexpectedTokenTransferError`] - the balances pallet threw an error during the attempted transfer /// #[pallet::call_index(14)] - #[pallet::weight(T::WeightInfo::withdraw_tokens())] + #[pallet::weight((T::WeightInfo::withdraw_tokens(), DispatchClass::Normal, Pays::No))] pub fn withdraw_tokens( origin: OriginFor, - msa_owner_public_key: T::AccountId, - msa_owner_proof: MultiSignature, - authorization_payload: AddKeyData, + _msa_owner_public_key: T::AccountId, + _msa_owner_proof: MultiSignature, + authorization_payload: AuthorizedKeyData, ) -> DispatchResult { let public_key = ensure_signed(origin)?; - ensure!(public_key == authorization_payload.new_public_key, Error::::NotKeyOwner); - - ensure!( - Self::verify_signature( - &msa_owner_proof, - &msa_owner_public_key, - &authorization_payload - ), - Error::::MsaOwnershipInvalidSignature - ); - - Self::register_signature(&msa_owner_proof, authorization_payload.expiration)?; - let msa_id = authorization_payload.msa_id; - Self::ensure_msa_owner(&msa_owner_public_key, msa_id)?; - // - Get account address for MSA let msa_address = Self::msa_id_to_eth_address(msa_id); @@ -990,23 +980,6 @@ pub mod pallet { Preservation::Expendable, Fortitude::Polite, ); - ensure!(msa_balance > Zero::zero(), Error::::InsufficientBalanceToWithdraw); - - // - TODO: Phase 2 (free transaction w/fee paid from proceeds): Check that MSA balance is > fee - - let ed = T::Currency::minimum_balance(); - // Make sure either: - // - Balance to be transferred > existential deposit, OR - // - Destination account balance + ammount to be transferred > existential deposit - let amount_alone_is_insufficient = msa_balance < ed; - let dest_will_be_reaped = if amount_alone_is_insufficient { - let dest_balance = T::Currency::total_balance(&public_key); - (dest_balance + msa_balance) < ed - } else { - amount_alone_is_insufficient - }; - - ensure!(!dest_will_be_reaped, Error::::InsufficientBalanceToFundDestination); let result = ::Currency::transfer( &msa_account_id, @@ -1940,6 +1913,120 @@ impl CheckFreeExtrinsicUse { ValidTransaction::with_tag_prefix(TAG_PREFIX).and_provides(account_id).build() } + + /// Validates that a request to withdraw tokens from an MSA passes the following checks: + /// - Receiver (origin) is NOT a control key for any MSA + /// - Signed payload matches MSA ID and signature validation + /// - Signature is not a duplicate + /// - MSA has tokens available to be withdrawn + /// - Receiver account has/will have enough balance to avoid being reaped + /// + /// # Errors + /// + /// * [`Error::NotKeyOwner`] - the transaction signer origin is not the owner of the public key contained in the provided `authorization_payload` + /// * [`Error::MsaOwnershipInvalidSignature`] - `msa_owner_public_key` is not a valid signer of the provided `authorization_payload`. + /// * [`Error::NoKeyExists`] - the public key supplied in `msa_owner_public_key` is not a registered control key of any MSA. + /// * [`Error::NotMsaOwner`] - the MSA ID in `authorization_payload` does not match the MSA ID of the `msa_owner_public_key`. + /// * [`Error::ProofHasExpired`] - the current block is less than the `expired` block number set in `AddKeyData`. + /// * [`Error::ProofNotYetValid`] - the `expired` block number set in `AddKeyData` is greater than the current block number plus mortality_block_limit(). + /// * [`Error::SignatureAlreadySubmitted`] - signature has already been used. + /// * [`Error::InsufficientBalanceToWithdraw`] - the MSA account has not balance to withdraw + /// * [`Error::InsufficientBalanceToFundDestination`] - the transfer would result in an account that would be reaped due to existential balance requirements + /// + pub fn validate_msa_token_withdrawal( + receiver_account_id: &T::AccountId, + msa_owner_public_key: &T::AccountId, + msa_owner_proof: &MultiSignature, + authorization_payload: &AuthorizedKeyData, + ) -> TransactionValidity { + const TAG_PREFIX: &str = "MsaTokenWithdrawal"; + + ensure!( + *receiver_account_id == authorization_payload.authorized_public_key, + InvalidTransaction::Custom(ValidityError::NotKeyOwner as u8) + ); + + ensure!( + Pallet::::verify_signature(&msa_owner_proof, &msa_owner_public_key, authorization_payload), + InvalidTransaction::Custom(ValidityError::MsaOwnershipInvalidSignature as u8) + ); + + // Origin must NOT be an MSA control key + ensure!( + !PublicKeyToMsaId::::contains_key(&receiver_account_id), + InvalidTransaction::Custom(ValidityError::IneligibleOrigin as u8) + ); + + Pallet::::register_signature(&msa_owner_proof, authorization_payload.expiration).map_err(|err: DispatchError| { + InvalidTransaction::Custom(match err { + DispatchError::Module(sp_runtime::ModuleError { error, ..}) => { + if let Ok(decoded_err) = pallet::Error::::decode(&mut &error[..]) { + match decoded_err { + pallet::Error::::ProofNotYetValid => ValidityError::ProofNotYetValid as u8, + pallet::Error::::ProofHasExpired => ValidityError::ProofHasExpired as u8, + pallet::Error::::SignatureAlreadySubmitted => ValidityError::SignatureAlreadySubmitted as u8, + _ => 255u8, + } + } else { + 255u8 + } + }, + _ => 255u8, + }) + })?; + + let msa_id = authorization_payload.msa_id; + + Pallet::::ensure_msa_owner(&msa_owner_public_key, msa_id).map_err(|err| { + InvalidTransaction::Custom(match err { + DispatchError::Module(sp_runtime::ModuleError {error, ..}) => { + if let Ok(decoded_err) = pallet::Error::::decode(&mut &error[..]) { + match decoded_err { + pallet::Error::::NoKeyExists => ValidityError::NoKeyExists as u8, + pallet::Error::::NotMsaOwner => ValidityError::NotMsaOwner as u8, + _ => 255u8, + } + } else { + 255u8 + } + }, + _ => 255u8 + }) + })?; + + // - Get account address for MSA + let msa_address = Pallet::::msa_id_to_eth_address(msa_id); + + // - Convert to AccountId + let mut bytes = &EthereumAddressMapper::to_bytes32(&msa_address.0)[..]; + let msa_account_id = T::AccountId::decode(&mut bytes).unwrap(); + + // - Check that the MSA has a balance to withdraw + let msa_balance = T::Currency::reducible_balance( + &msa_account_id, + Preservation::Expendable, + Fortitude::Polite, + ); + ensure!(msa_balance > Zero::zero(), InvalidTransaction::Custom(ValidityError::InsufficientBalanceToWithdraw as u8)); + + // - TODO: Phase 2 (free transaction w/fee paid from proceeds): Check that MSA balance is > fee + + let ed = T::Currency::minimum_balance(); + // Make sure either: + // - Balance to be transferred > existential deposit, OR + // - Destination account balance + ammount to be transferred > existential deposit + let amount_alone_is_insufficient = msa_balance < ed; + let dest_will_be_reaped = if amount_alone_is_insufficient { + let dest_balance = T::Currency::total_balance(&receiver_account_id); + (dest_balance + msa_balance) < ed + } else { + false + }; + + ensure!(!dest_will_be_reaped, InvalidTransaction::Custom(ValidityError::InsufficientBalanceToFundDestination as u8)); + + ValidTransaction::with_tag_prefix(TAG_PREFIX).and_provides(receiver_account_id).build() + } } /// Errors related to the validity of the CheckFreeExtrinsicUse signed extension. @@ -1960,6 +2047,26 @@ pub enum ValidityError { InvalidNonZeroProviderDelegations, /// HandleNotRetired HandleNotRetired, + /// Cryptographic signature verification failed + MsaOwnershipInvalidSignature, + /// MSA balance is zero (paid transaction) or insufficient to cover fees (free transaction) + InsufficientBalanceToWithdraw, + /// MSA balance to withdraw is insufficient to fund the destination account such that it would not be reaped + InsufficientBalanceToFundDestination, + /// Fund transfer error + UnexpectedTokenTransferError, + /// Origin is ineleligible for the current transaction + IneligibleOrigin, + /// Proof not yet valid + ProofNotYetValid, + /// Proof has expired + ProofHasExpired, + /// Duplicate signature + SignatureAlreadySubmitted, + /// No key exists + NoKeyExists, + /// Not MSA owner + NotMsaOwner } impl CheckFreeExtrinsicUse { @@ -2036,6 +2143,16 @@ where Some(Call::delete_msa_public_key { public_key_to_delete, .. }) => CheckFreeExtrinsicUse::::validate_key_delete(who, public_key_to_delete), Some(Call::retire_msa { .. }) => CheckFreeExtrinsicUse::::ensure_msa_can_retire(who), + Some(Call::withdraw_tokens { + msa_owner_public_key, + msa_owner_proof, + authorization_payload, + }) => CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + who, + msa_owner_public_key, + msa_owner_proof, + authorization_payload, + ), _ => Ok(Default::default()), } } diff --git a/pallets/msa/src/tests/msa_token_tests.rs b/pallets/msa/src/tests/msa_token_tests.rs index 21b4c1f36e..38032d7fa2 100644 --- a/pallets/msa/src/tests/msa_token_tests.rs +++ b/pallets/msa/src/tests/msa_token_tests.rs @@ -1,15 +1,14 @@ use frame_support::{ - assert_noop, assert_ok, - traits::{ + assert_err, assert_noop, assert_ok, pallet_prelude::InvalidTransaction, traits::{ tokens::{fungible::Inspect, Fortitude, Preservation}, Currency, - }, + } }; use sp_core::{sr25519, Encode, Pair}; use sp_runtime::MultiSignature; -use crate::{tests::mock::*, types::AddKeyData, Config, Error}; +use crate::{tests::mock::*, types::AuthorizedKeyData, CheckFreeExtrinsicUse, Config, Error, ValidityError}; use common_primitives::{ msa::{MessageSourceId, H160}, @@ -23,16 +22,16 @@ use pallet_balances::Event as BalancesEvent; fn generate_payload( msa_id: MessageSourceId, msa_owner_keys: &sr25519::Pair, - new_public_key: &sr25519::Pair, + authorized_public_key: &sr25519::Pair, expiration: Option, -) -> (AddKeyData, Vec, MultiSignature) { - let payload = AddKeyData:: { +) -> (AuthorizedKeyData, Vec, MultiSignature) { + let payload = AuthorizedKeyData:: { msa_id, expiration: match expiration { Some(block_number) => block_number, None => 10, }, - new_public_key: new_public_key.public().into(), + authorized_public_key: authorized_public_key.public().into(), }; let encoded_payload = wrap_binary_data(payload.encode()); @@ -52,13 +51,13 @@ fn it_fails_when_caller_key_does_not_match_payload() { generate_payload(msa_id, &owner_key_pair, &other_key_pair, None); assert_noop!( - Msa::withdraw_tokens( - RuntimeOrigin::signed(origin_key_pair.public().into()), - owner_key_pair.public().into(), - msa_signature, - payload + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &owner_key_pair.public().into(), + &msa_signature, + &payload ), - Error::::NotKeyOwner + InvalidTransaction::Custom(ValidityError::NotKeyOwner as u8) ); }); } @@ -74,13 +73,13 @@ fn it_fails_when_payload_signature_is_invalid() { generate_payload(msa_id, &other_key_pair, &origin_key_pair, None); assert_noop!( - Msa::withdraw_tokens( - RuntimeOrigin::signed(origin_key_pair.public().into()), - owner_key_pair.public().into(), - msa_signature, - payload + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &owner_key_pair.public().into(), + &msa_signature, + &payload ), - Error::::MsaOwnershipInvalidSignature + InvalidTransaction::Custom(ValidityError::MsaOwnershipInvalidSignature as u8) ); }); } @@ -97,13 +96,13 @@ fn it_fails_when_proof_is_expired() { generate_payload(msa_id, &owner_key_pair, &origin_key_pair, Some(1)); assert_noop!( - Msa::withdraw_tokens( - RuntimeOrigin::signed(origin_key_pair.public().into()), - owner_key_pair.public().into(), - msa_signature, - payload + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &owner_key_pair.public().into(), + &msa_signature, + &payload ), - Error::::ProofHasExpired + InvalidTransaction::Custom(ValidityError::ProofHasExpired as u8) ); }); } @@ -124,13 +123,34 @@ fn it_fails_when_proof_is_not_yet_valid() { ); assert_noop!( - Msa::withdraw_tokens( - RuntimeOrigin::signed(origin_key_pair.public().into()), - owner_key_pair.public().into(), - msa_signature, - payload + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &owner_key_pair.public().into(), + &msa_signature, + &payload ), - Error::::ProofNotYetValid + InvalidTransaction::Custom(ValidityError::ProofNotYetValid as u8) + ); + }); +} + +#[test] +fn it_fails_when_origin_is_an_msa_control_key() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (_, origin_key_pair) = create_account(); + + let (payload, _, msa_signature) = + generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); + + assert_noop!( + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &owner_key_pair.public().into(), + &msa_signature, + &payload + ), + InvalidTransaction::Custom(ValidityError::IneligibleOrigin as u8) ); }); } @@ -144,14 +164,14 @@ fn it_fails_when_msa_key_is_not_an_msa_control_key() { let (payload, _, msa_signature) = generate_payload(msa_id + 1, &owner_key_pair, &origin_key_pair, None); - assert_noop!( - Msa::withdraw_tokens( - RuntimeOrigin::signed(origin_key_pair.public().into()), - owner_key_pair.public().into(), - msa_signature, - payload + assert_err!( + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &owner_key_pair.public().into(), + &msa_signature, + &payload ), - Error::::NotMsaOwner + InvalidTransaction::Custom(ValidityError::NotMsaOwner as u8) ); }) } @@ -166,14 +186,14 @@ fn it_fails_when_msa_key_does_not_control_msa_in_payload() { let (payload, _, msa_signature) = generate_payload(msa_id, &other_key_pair, &origin_key_pair, None); - assert_noop!( - Msa::withdraw_tokens( - RuntimeOrigin::signed(origin_key_pair.public().into()), - other_key_pair.public().into(), - msa_signature, - payload + assert_err!( + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &other_key_pair.public().into(), + &msa_signature, + &payload ), - Error::::NoKeyExists + InvalidTransaction::Custom(ValidityError::NoKeyExists as u8) ); }) } @@ -187,14 +207,14 @@ fn it_fails_when_msa_does_not_have_a_balance() { let (payload, _, msa_signature) = generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); - assert_noop!( - Msa::withdraw_tokens( - RuntimeOrigin::signed(origin_key_pair.public().into()), - owner_key_pair.public().into(), - msa_signature, - payload + assert_err!( + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &owner_key_pair.public().into(), + &msa_signature, + &payload ), - Error::::InsufficientBalanceToWithdraw + InvalidTransaction::Custom(ValidityError::InsufficientBalanceToWithdraw as u8) ); }) } diff --git a/pallets/msa/src/tests/other_tests.rs b/pallets/msa/src/tests/other_tests.rs index a931da3011..b5fa27f8a4 100644 --- a/pallets/msa/src/tests/other_tests.rs +++ b/pallets/msa/src/tests/other_tests.rs @@ -13,8 +13,8 @@ use crate::{ ensure, tests::mock::*, types::{AddProvider, PermittedDelegationSchemas, EMPTY_FUNCTION}, - AddKeyData, Config, DelegatorAndProviderToDelegation, DispatchResult, Error, Event, - ProviderToRegistryEntry, PublicKeyToMsaId, + AddKeyData, AuthorizedKeyData, Config, DelegatorAndProviderToDelegation, DispatchResult, Error, + Event, ProviderToRegistryEntry, PublicKeyToMsaId, }; use common_primitives::signatures::AccountAddressMapper; @@ -789,6 +789,7 @@ fn eth_address_to_checksummed_string() { } } +// Test data generated by [this tool](../../eth-migration/metamask.html) #[test] fn ethereum_eip712_signatures_for_add_key_should_work() { new_test_ext().execute_with(|| { @@ -819,6 +820,36 @@ fn ethereum_eip712_signatures_for_add_key_should_work() { }); } +#[test] +fn ethereum_eip712_signatures_for_authorized_key_should_work() { + new_test_ext().execute_with(|| { + let address = EthereumAddressMapper::to_account_id(&from_hex("0x7A23F8D62589aB9651722C7F4a0E998D7d3Ef2A9").unwrap_or_default()); + let payload: AuthorizedKeyData = AuthorizedKeyData { + msa_id: 12876327, + expiration: 100, + authorized_public_key: address.into(), + }; + let encoded_payload = payload.encode_eip_712(); + + // following signature is generated via Metamask using the same input to check compatibility + let signature_raw = from_hex("0x9dec03e5c93e2b9619cc8fd77383a8fc38d4aa3dc20fa26be436d386acb380260e2a82e677b71f28adc7cc63b60855ccc481057307a1b05dbb2f5af19c66b5461c").expect("Should convert"); + let unified_signature = UnifiedSignature::from(ecdsa::Signature::from_raw( + signature_raw.try_into().expect("should convert"), + )); + + // Non-compressed public key associated with the keypair used in Metamask + // 0x509540919faacf9ab52146c9aa40db68172d83777250b28e4679176e49ccdd9fa213197dc0666e85529d6c9dda579c1295d61c417f01505765481e89a4016f02 + let public_key = ecdsa::Public::from_raw( + from_hex("0x02509540919faacf9ab52146c9aa40db68172d83777250b28e4679176e49ccdd9f") + .expect("should convert") + .try_into() + .expect("invalid size"), + ); + let unified_signer = UnifiedSigner::from(public_key); + assert!(unified_signature.verify(&encoded_payload[..], &unified_signer.into_account())); + }); +} + #[test] fn ethereum_eip712_signatures_for_add_provider_should_work() { new_test_ext().execute_with(|| { diff --git a/pallets/msa/src/types.rs b/pallets/msa/src/types.rs index a0a0bd52dd..d22604310f 100644 --- a/pallets/msa/src/types.rs +++ b/pallets/msa/src/types.rs @@ -21,9 +21,8 @@ use sp_core::U256; /// Dispatch Empty pub const EMPTY_FUNCTION: fn(MessageSourceId) -> DispatchResult = |_| Ok(()); -/// A type definition for the payload for the following operations: +/// A type definition for the payload for the following operation: /// - Adding an MSA key - `pallet_msa::add_public_key_to_msa` -/// - Authorizing a token withdrawal to an address associated with a public key - `pallet_msa::withdraw_tokens` #[derive( TypeInfo, RuntimeDebugNoBound, Clone, Decode, DecodeWithMemTracking, Encode, PartialEq, Eq, )] @@ -70,6 +69,55 @@ impl EIP712Encode for AddKeyData { } } +/// A type definition for the payload for authorizing a public key for the following operations: +/// - Authorizing a token withdrawal to an address associated with a public key - `pallet_msa::withdraw_tokens` +#[derive( + TypeInfo, RuntimeDebugNoBound, Clone, Decode, DecodeWithMemTracking, Encode, PartialEq, Eq, +)] +#[scale_info(skip_type_params(T))] +pub struct AuthorizedKeyData { + /// Message Source Account identifier + pub msa_id: MessageSourceId, + /// The block number at which a signed proof of this payload expires. + pub expiration: BlockNumberFor, + /// The public key to be added that is authorized as the target of the current operation + pub authorized_public_key: T::AccountId, +} + +impl EIP712Encode for AuthorizedKeyData { + fn encode_eip_712(&self) -> Box<[u8]> { + lazy_static! { + // get prefix and domain separator + static ref PREFIX_DOMAIN_SEPARATOR: Box<[u8]> = + get_eip712_encoding_prefix("0xcccccccccccccccccccccccccccccccccccccccc"); + + // signed payload + static ref MAIN_TYPE_HASH: [u8; 32] = sp_io::hashing::keccak_256( + b"AuthorizedKeyData(uint64 msaId,uint32 expiration,address authorizedPublicKey)", + ); + } + let coded_owner_msa_id = to_abi_compatible_number(self.msa_id); + let expiration: U256 = self.expiration.into(); + let coded_expiration = to_abi_compatible_number(expiration.as_u128()); + let converted_public_key = + T::ConvertIntoAccountId32::convert(self.authorized_public_key.clone()); + let mut zero_prefixed_address = [0u8; 32]; + zero_prefixed_address[12..] + .copy_from_slice(&EthereumAddressMapper::to_ethereum_address(converted_public_key).0); + let message = sp_io::hashing::keccak_256( + &[ + MAIN_TYPE_HASH.as_slice(), + &coded_owner_msa_id, + &coded_expiration, + &zero_prefixed_address, + ] + .concat(), + ); + let combined = [PREFIX_DOMAIN_SEPARATOR.as_ref(), &message].concat(); + combined.into_boxed_slice() + } +} + /// Structure that is signed for granting permissions to a Provider #[derive(TypeInfo, Clone, Debug, Decode, DecodeWithMemTracking, Encode, PartialEq, Eq)] pub struct AddProvider { diff --git a/tools/eth-migration/metamask.html b/tools/eth-migration/metamask.html index fe4e147468..25140b079a 100644 --- a/tools/eth-migration/metamask.html +++ b/tools/eth-migration/metamask.html @@ -331,6 +331,51 @@ alert("Signature: " + signature) }); + document.getElementById('authorized_key_payload')?.addEventListener("click", async function () { + const signature_request = { + "types": { + ...eip712domain_type_definition, + "AuthorizedKeyData": [ + { + "name": "msaId", + "type": "uint64" + }, + { + "name": "expiration", + "type": "uint32" + }, + { + "name": "authorizedPublicKey", + "type": "address" + }, + ], + }, + "primaryType": "AuthorizedKeyData", + "domain": karma_request_domain, + "message": { + "msaId": 12876327, + "expiration": 100, + "authorizedPublicKey": '0x7A23F8D62589aB9651722C7F4a0E998D7d3Ef2A9', + } + } + console.log(JSON.stringify({ + "method": "eth_signTypedData_v4", + "params": [ + accounts[0], + signature_request + ] + },null, 2)); + let signature = await window.ethereum.request({ + "method": "eth_signTypedData_v4", + "params": [ + accounts[0], + signature_request + ] + }) + console.log(signature); + alert("Signature: " + signature) + }); + document.getElementById('add_provider_request')?.addEventListener("click", async function () { const signature_request = { "types": { @@ -391,5 +436,6 @@

+ From a09efa1347e6cdc73b2a4f2c339d0eab3824ac46 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Wed, 21 May 2025 13:56:12 -0400 Subject: [PATCH 12/19] fix: formatting --- pallets/msa/src/lib.rs | 62 +++++++++++++++--------- pallets/msa/src/tests/msa_token_tests.rs | 10 ++-- 2 files changed, 46 insertions(+), 26 deletions(-) diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index 1a0f67e4a0..69a795a894 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -1947,7 +1947,11 @@ impl CheckFreeExtrinsicUse { ); ensure!( - Pallet::::verify_signature(&msa_owner_proof, &msa_owner_public_key, authorization_payload), + Pallet::::verify_signature( + &msa_owner_proof, + &msa_owner_public_key, + authorization_payload + ), InvalidTransaction::Custom(ValidityError::MsaOwnershipInvalidSignature as u8) ); @@ -1957,29 +1961,33 @@ impl CheckFreeExtrinsicUse { InvalidTransaction::Custom(ValidityError::IneligibleOrigin as u8) ); - Pallet::::register_signature(&msa_owner_proof, authorization_payload.expiration).map_err(|err: DispatchError| { - InvalidTransaction::Custom(match err { - DispatchError::Module(sp_runtime::ModuleError { error, ..}) => { - if let Ok(decoded_err) = pallet::Error::::decode(&mut &error[..]) { - match decoded_err { - pallet::Error::::ProofNotYetValid => ValidityError::ProofNotYetValid as u8, - pallet::Error::::ProofHasExpired => ValidityError::ProofHasExpired as u8, - pallet::Error::::SignatureAlreadySubmitted => ValidityError::SignatureAlreadySubmitted as u8, - _ => 255u8, + Pallet::::register_signature(&msa_owner_proof, authorization_payload.expiration) + .map_err(|err: DispatchError| { + InvalidTransaction::Custom(match err { + DispatchError::Module(sp_runtime::ModuleError { error, .. }) => { + if let Ok(decoded_err) = pallet::Error::::decode(&mut &error[..]) { + match decoded_err { + pallet::Error::::ProofNotYetValid => + ValidityError::ProofNotYetValid as u8, + pallet::Error::::ProofHasExpired => + ValidityError::ProofHasExpired as u8, + pallet::Error::::SignatureAlreadySubmitted => + ValidityError::SignatureAlreadySubmitted as u8, + _ => 255u8, + } + } else { + 255u8 } - } else { - 255u8 - } - }, - _ => 255u8, - }) - })?; + }, + _ => 255u8, + }) + })?; let msa_id = authorization_payload.msa_id; Pallet::::ensure_msa_owner(&msa_owner_public_key, msa_id).map_err(|err| { InvalidTransaction::Custom(match err { - DispatchError::Module(sp_runtime::ModuleError {error, ..}) => { + DispatchError::Module(sp_runtime::ModuleError { error, .. }) => { if let Ok(decoded_err) = pallet::Error::::decode(&mut &error[..]) { match decoded_err { pallet::Error::::NoKeyExists => ValidityError::NoKeyExists as u8, @@ -1990,7 +1998,7 @@ impl CheckFreeExtrinsicUse { 255u8 } }, - _ => 255u8 + _ => 255u8, }) })?; @@ -2007,7 +2015,10 @@ impl CheckFreeExtrinsicUse { Preservation::Expendable, Fortitude::Polite, ); - ensure!(msa_balance > Zero::zero(), InvalidTransaction::Custom(ValidityError::InsufficientBalanceToWithdraw as u8)); + ensure!( + msa_balance > Zero::zero(), + InvalidTransaction::Custom(ValidityError::InsufficientBalanceToWithdraw as u8) + ); // - TODO: Phase 2 (free transaction w/fee paid from proceeds): Check that MSA balance is > fee @@ -2023,9 +2034,14 @@ impl CheckFreeExtrinsicUse { false }; - ensure!(!dest_will_be_reaped, InvalidTransaction::Custom(ValidityError::InsufficientBalanceToFundDestination as u8)); + ensure!( + !dest_will_be_reaped, + InvalidTransaction::Custom(ValidityError::InsufficientBalanceToFundDestination as u8) + ); - ValidTransaction::with_tag_prefix(TAG_PREFIX).and_provides(receiver_account_id).build() + ValidTransaction::with_tag_prefix(TAG_PREFIX) + .and_provides(receiver_account_id) + .build() } } @@ -2066,7 +2082,7 @@ pub enum ValidityError { /// No key exists NoKeyExists, /// Not MSA owner - NotMsaOwner + NotMsaOwner, } impl CheckFreeExtrinsicUse { diff --git a/pallets/msa/src/tests/msa_token_tests.rs b/pallets/msa/src/tests/msa_token_tests.rs index 38032d7fa2..376f25d04f 100644 --- a/pallets/msa/src/tests/msa_token_tests.rs +++ b/pallets/msa/src/tests/msa_token_tests.rs @@ -1,14 +1,18 @@ use frame_support::{ - assert_err, assert_noop, assert_ok, pallet_prelude::InvalidTransaction, traits::{ + assert_err, assert_noop, assert_ok, + pallet_prelude::InvalidTransaction, + traits::{ tokens::{fungible::Inspect, Fortitude, Preservation}, Currency, - } + }, }; use sp_core::{sr25519, Encode, Pair}; use sp_runtime::MultiSignature; -use crate::{tests::mock::*, types::AuthorizedKeyData, CheckFreeExtrinsicUse, Config, Error, ValidityError}; +use crate::{ + tests::mock::*, types::AuthorizedKeyData, CheckFreeExtrinsicUse, Config, Error, ValidityError, +}; use common_primitives::{ msa::{MessageSourceId, H160}, From 592ea06f6722574e6594e4587de90c776c4c5b12 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Wed, 21 May 2025 14:37:46 -0400 Subject: [PATCH 13/19] fix: lint --- pallets/msa/src/lib.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index 69a795a894..0fe9fd7396 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -940,7 +940,7 @@ pub mod pallet { /// It also contains an expiration block number for the proof, ensuring it is valid and must be greater than the current block. /// /// # Events - /// * [`pallet_balances::Event::::Transfer`] - Transfer token event + /// * `pallet_balances::Event::::Transfer` - Transfer token event /// /// # Errors /// @@ -953,7 +953,6 @@ pub mod pallet { /// * [`Error::SignatureAlreadySubmitted`] - signature has already been used. /// * [`Error::InsufficientBalanceToWithdraw`] - the MSA account has not balance to withdraw /// * [`Error::InsufficientBalanceToFundDestination`] - the transfer would result in an account that would be reaped due to existential balance requirements - /// * [`Error::UnexpectedTokenTransferError`] - the balances pallet threw an error during the attempted transfer /// #[pallet::call_index(14)] #[pallet::weight((T::WeightInfo::withdraw_tokens(), DispatchClass::Normal, Pays::No))] @@ -1948,8 +1947,8 @@ impl CheckFreeExtrinsicUse { ensure!( Pallet::::verify_signature( - &msa_owner_proof, - &msa_owner_public_key, + msa_owner_proof, + msa_owner_public_key, authorization_payload ), InvalidTransaction::Custom(ValidityError::MsaOwnershipInvalidSignature as u8) @@ -1957,11 +1956,11 @@ impl CheckFreeExtrinsicUse { // Origin must NOT be an MSA control key ensure!( - !PublicKeyToMsaId::::contains_key(&receiver_account_id), + !PublicKeyToMsaId::::contains_key(receiver_account_id), InvalidTransaction::Custom(ValidityError::IneligibleOrigin as u8) ); - Pallet::::register_signature(&msa_owner_proof, authorization_payload.expiration) + Pallet::::register_signature(msa_owner_proof, authorization_payload.expiration) .map_err(|err: DispatchError| { InvalidTransaction::Custom(match err { DispatchError::Module(sp_runtime::ModuleError { error, .. }) => { @@ -1985,7 +1984,7 @@ impl CheckFreeExtrinsicUse { let msa_id = authorization_payload.msa_id; - Pallet::::ensure_msa_owner(&msa_owner_public_key, msa_id).map_err(|err| { + Pallet::::ensure_msa_owner(msa_owner_public_key, msa_id).map_err(|err| { InvalidTransaction::Custom(match err { DispatchError::Module(sp_runtime::ModuleError { error, .. }) => { if let Ok(decoded_err) = pallet::Error::::decode(&mut &error[..]) { @@ -2028,7 +2027,7 @@ impl CheckFreeExtrinsicUse { // - Destination account balance + ammount to be transferred > existential deposit let amount_alone_is_insufficient = msa_balance < ed; let dest_will_be_reaped = if amount_alone_is_insufficient { - let dest_balance = T::Currency::total_balance(&receiver_account_id); + let dest_balance = T::Currency::total_balance(receiver_account_id); (dest_balance + msa_balance) < ed } else { false From a5043536b68a0f2f1dd98f75461eb2b352b592c1 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Thu, 22 May 2025 16:14:05 -0400 Subject: [PATCH 14/19] fix: signature registry checks in extension, e2e nonce issues --- e2e/msa/msaTokens.test.ts | 96 +++++++++------- e2e/scaffolding/extrinsicHelpers.ts | 2 +- pallets/msa/src/lib.rs | 134 +++++++---------------- pallets/msa/src/tests/msa_token_tests.rs | 54 ++++++++- 4 files changed, 151 insertions(+), 135 deletions(-) diff --git a/e2e/msa/msaTokens.test.ts b/e2e/msa/msaTokens.test.ts index 116e6aac6a..1f82e85e5d 100644 --- a/e2e/msa/msaTokens.test.ts +++ b/e2e/msa/msaTokens.test.ts @@ -21,6 +21,7 @@ import { u64 } from '@polkadot/types'; import { Codec } from '@polkadot/types/types'; const fundingSource = getFundingSource(import.meta.url); +const TRANSFER_AMOUNT = 1n * DOLLARS; /** * @@ -92,13 +93,12 @@ describe('MSAs Holding Tokens', function () { describe('Send tokens to MSA', function () { it('should send tokens to the MSA', async function () { const ed = getExistentialDeposit(); - const transferAmount = 1n + ed; let accountData = await ExtrinsicHelper.getAccountInfo(ethKeys); const initialBalance = accountData.data.free.toBigInt(); const op = ExtrinsicHelper.transferFunds( fundingSource, ethereumAddressToKeyringPair(ethAddress20), - transferAmount + TRANSFER_AMOUNT ); const { target: transferEvent } = await op.fundAndSend(fundingSource); @@ -108,7 +108,7 @@ describe('MSAs Holding Tokens', function () { const finalBalance = accountData.data.free.toBigInt(); assert.equal( finalBalance, - initialBalance + transferAmount, + initialBalance + TRANSFER_AMOUNT, 'Final balance should be increased by transfer amount' ); }); @@ -160,7 +160,7 @@ describe('MSAs Holding Tokens', function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', badPayload); ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, badPayload); - await assert.rejects(op.signAndSend(), { + await assert.rejects(op.signAndSend('current'), { name: 'RpcError', code: 1010, data: 'Custom error: 5', // NotKeyOwner, @@ -171,14 +171,14 @@ describe('MSAs Holding Tokens', function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); badSig = signPayloadSr25519(createKeys(), authorizedKeyData); // Invalid signature const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, badSig, payload); - await assert.rejects(op.signAndSend(), { + await assert.rejects(op.signAndSend('current'), { name: 'RpcError', code: 1010, data: 'Custom error: 8', // MsaOwnershipInvalidSignature }); }); - it('should fail if expiration has passed (ProofHasExpired)', async function () { + it('should fail if expiration has passed (MsaOwnershipInvalidSignature)', async function () { const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber(), @@ -186,14 +186,14 @@ describe('MSAs Holding Tokens', function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); - await assert.rejects(op.signAndSend(), { + await assert.rejects(op.signAndSend('current'), { name: 'RpcError', code: 1010, - data: 'Custom error: 14', // ProofHasExpired, + data: 'Custom error: 8', // MsaOwnershipInvalidSignature, }); }); - it('should fail if expiration is not yet valid (ProofNotYetValid)', async function () { + it('should fail if expiration is not yet valid (MsaOwnershipInvalidSignature)', async function () { const maxMortality = ExtrinsicHelper.api.consts.msa.mortalityWindowSize.toNumber(); const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, @@ -202,10 +202,10 @@ describe('MSAs Holding Tokens', function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); - await assert.rejects(op.signAndSend(), { + await assert.rejects(op.signAndSend('current'), { name: 'RpcError', code: 1010, - data: 'Custom error: 13', // ProofNotYetValid, + data: 'Custom error: 8', // MsaOwnershipInvalidSignature, }); }); @@ -218,14 +218,14 @@ describe('MSAs Holding Tokens', function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); const op = ExtrinsicHelper.withdrawTokens(otherMsaKeys, msaKeys, ownerSig, newPayload); - await assert.rejects(op.signAndSend(), { + await assert.rejects(op.signAndSend('current'), { name: 'RpcError', code: 1010, - data: 'Custom error: 12', // IneligibleOrigin, + data: 'Custom error: 10', // IneligibleOrigin, }); }); - it('should fail if payload signer does not control the MSA in the signed payload (NotMsaOwner)', async function () { + it('should fail if payload signer does not control the MSA in the signed payload (InvalidMsaKey)', async function () { const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, msaId: new u64(ExtrinsicHelper.api.registry, 9999), @@ -233,14 +233,14 @@ describe('MSAs Holding Tokens', function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); - await assert.rejects(op.signAndSend(), { + await assert.rejects(op.signAndSend('current'), { name: 'RpcError', code: 1010, - data: 'Custom error: 17', // NotMsaOwner, + data: 'Custom error: 1', // InvalidMsaKey, }); }); - it('should fail if payload signer is not an MSA control key (NoKeyExists)', async function () { + it('should fail if payload signer is not an MSA control key (InvalidMsaKey)', async function () { const badSigner = createKeys(); const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, @@ -249,10 +249,10 @@ describe('MSAs Holding Tokens', function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); ownerSig = signPayloadSr25519(badSigner, authorizedKeyData); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, badSigner, ownerSig, newPayload); - await assert.rejects(op.signAndSend(), { + await assert.rejects(op.signAndSend('current'), { name: 'RpcError', code: 1010, - data: 'Custom error: 16', // NoKeyExists, + data: 'Custom error: 1', // InvalidMsaKey, }); }); @@ -260,7 +260,7 @@ describe('MSAs Holding Tokens', function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); - await assert.rejects(op.signAndSend(), { + await assert.rejects(op.signAndSend('current'), { name: 'RpcError', code: 1010, data: 'Custom error: 9', // InsufficientBalanceToWithdraw, @@ -268,37 +268,59 @@ describe('MSAs Holding Tokens', function () { }); it('should succeed', async function () { - const transferAmount = 1n * DOLLARS; const { data: { free: startingBalance }, } = await ExtrinsicHelper.getAccountInfo(secondaryKey); - // Send tokens to MSA - try { - const { target: transferEvent } = await ExtrinsicHelper.transferFunds( - fundingSource, - ethereumAddressToKeyringPair(msaAddress), - transferAmount - ).signAndSend(); - assert.notEqual(transferEvent, undefined, 'should have transferred tokens to MSA'); - } catch (err: any) { - console.error('Error sending tokens to MSA', err.message); - } - + const op1 = ExtrinsicHelper.transferFunds( + fundingSource, + ethereumAddressToKeyringPair(msaAddress), + TRANSFER_AMOUNT + ); + await assert.doesNotReject(op1.signAndSend(), 'MSA funding failed'); authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); - const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); - await assert.doesNotReject(op.signAndSend(), 'token transfer transaction should have succeeded'); - + const op2 = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); + await assert.doesNotReject(op2.signAndSend('current'), 'token transfer transaction should have succeeded'); // Destination account should have had balance increased const { data: { free: endingBalance }, } = await ExtrinsicHelper.getAccountInfo(secondaryKey); assert( - startingBalance.toBigInt() + transferAmount === endingBalance.toBigInt(), + startingBalance.toBigInt() + TRANSFER_AMOUNT === endingBalance.toBigInt(), 'balance of recieve should have increased by the transfer amount minus fee' ); }); + + it('should fail for duplicate signature submission (MsaOwnershipInvalidSignature)', async function () { + // Due to the fact that we're testing this free extrinsic with unfunded accounts (ie, with no nonce), + // we'll use a unique keypair for this test so that it doesn't matter whether other tests using + // the same keypair ended up funding the account & therefore initializing a nonce. + const keys = createKeys(); + payload.authorizedPublicKey = getUnifiedPublicKey(keys); + + const op1 = ExtrinsicHelper.transferFunds( + fundingSource, + ethereumAddressToKeyringPair(msaAddress), + TRANSFER_AMOUNT + ); + await assert.doesNotReject(op1.signAndSend(), 'MSA funding failed'); + + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); + ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + let op2 = ExtrinsicHelper.withdrawTokens(keys, msaKeys, ownerSig, payload); + await assert.doesNotReject(op2.signAndSend('current'), 'token withdrawal should have succeeded'); + + // Re-fund MSA so we don't fail for that + await assert.doesNotReject(op1.signAndSend(), 'MSA re-funding failed'); + + op2 = ExtrinsicHelper.withdrawTokens(keys, msaKeys, ownerSig, payload); + await assert.rejects(op2.signAndSend(0), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 8', // MsaOwnershipInvalidSignature, + }); + }); }); }); diff --git a/e2e/scaffolding/extrinsicHelpers.ts b/e2e/scaffolding/extrinsicHelpers.ts index d9a687da70..9489e0a085 100644 --- a/e2e/scaffolding/extrinsicHelpers.ts +++ b/e2e/scaffolding/extrinsicHelpers.ts @@ -193,7 +193,7 @@ export class Extrinsic { diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index 0fe9fd7396..992bd894ce 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -399,17 +399,8 @@ pub mod pallet { /// Attempted to add a new signature to a corrupt signature registry SignatureRegistryCorrupted, - /// MSA balance is zero (paid transaction) or insufficient to cover fees (free transaction) - InsufficientBalanceToWithdraw, - - /// MSA balance to withdraw is insufficient to fund the destination account such that it would not be reaped - InsufficientBalanceToFundDestination, - /// Fund transfer error UnexpectedTokenTransferError, - - /// Origin is ineleligible for the current transaction - IneligibleOrigin, } impl BlockNumberProvider for Pallet { @@ -959,10 +950,11 @@ pub mod pallet { pub fn withdraw_tokens( origin: OriginFor, _msa_owner_public_key: T::AccountId, - _msa_owner_proof: MultiSignature, + msa_owner_proof: MultiSignature, authorization_payload: AuthorizedKeyData, ) -> DispatchResult { let public_key = ensure_signed(origin)?; + Self::register_signature(&msa_owner_proof, authorization_payload.expiration)?; let msa_id = authorization_payload.msa_id; @@ -972,14 +964,12 @@ pub mod pallet { // - Convert to AccountId let mut bytes = &EthereumAddressMapper::to_bytes32(&msa_address.0)[..]; let msa_account_id = T::AccountId::decode(&mut bytes).unwrap(); - // - Check that the MSA has a balance to withdraw let msa_balance = T::Currency::reducible_balance( &msa_account_id, Preservation::Expendable, Fortitude::Polite, ); - let result = ::Currency::transfer( &msa_account_id, &public_key, @@ -1525,7 +1515,24 @@ impl Pallet { signature: &MultiSignature, signature_expires_at: BlockNumberFor, ) -> DispatchResult { - let current_block = frame_system::Pallet::::block_number(); + let current_block = Self::check_signature(signature, signature_expires_at)?; + + Self::enqueue_signature(signature, signature_expires_at, current_block) + } + + /// Check that mortality_block is within bounds. + /// Raises `SignatureAlreadySubmitted` if the signature exists in the registry. + /// + /// # Errors + /// * [`Error::ProofNotYetValid`] + /// * [`Error::ProofHasExpired`] + /// * [`Error::SignatureAlreadySubmitted`] + /// + pub fn check_signature( + signature: &MultiSignature, + signature_expires_at: BlockNumberFor, + ) -> Result, DispatchError> { + let current_block: BlockNumberFor = frame_system::Pallet::::block_number(); let max_lifetime = Self::mortality_block_limit(current_block); ensure!(max_lifetime > signature_expires_at, Error::::ProofNotYetValid); @@ -1536,8 +1543,11 @@ impl Pallet { !>::contains_key(signature), Error::::SignatureAlreadySubmitted ); + if let Some(signature_pointer) = PayloadSignatureRegistryPointer::::get() { + ensure!(signature_pointer.newest != *signature, Error::::SignatureAlreadySubmitted); + } - Self::enqueue_signature(signature, signature_expires_at, current_block) + Ok(current_block) } /// Do the actual enqueuing into the list storage and update the pointer @@ -1585,7 +1595,7 @@ impl Pallet { } // Add the newest signature if we are not the first - if pointer.count != 0 { + if pointer.count != 0 && current_block.le(&pointer.newest_expires_at) { >::insert( pointer.newest, (pointer.newest_expires_at, signature.clone()), @@ -1960,46 +1970,14 @@ impl CheckFreeExtrinsicUse { InvalidTransaction::Custom(ValidityError::IneligibleOrigin as u8) ); - Pallet::::register_signature(msa_owner_proof, authorization_payload.expiration) - .map_err(|err: DispatchError| { - InvalidTransaction::Custom(match err { - DispatchError::Module(sp_runtime::ModuleError { error, .. }) => { - if let Ok(decoded_err) = pallet::Error::::decode(&mut &error[..]) { - match decoded_err { - pallet::Error::::ProofNotYetValid => - ValidityError::ProofNotYetValid as u8, - pallet::Error::::ProofHasExpired => - ValidityError::ProofHasExpired as u8, - pallet::Error::::SignatureAlreadySubmitted => - ValidityError::SignatureAlreadySubmitted as u8, - _ => 255u8, - } - } else { - 255u8 - } - }, - _ => 255u8, - }) - })?; + Pallet::::check_signature(msa_owner_proof, authorization_payload.expiration).map_err( + |_| InvalidTransaction::Custom(ValidityError::MsaOwnershipInvalidSignature as u8), + )?; let msa_id = authorization_payload.msa_id; - Pallet::::ensure_msa_owner(msa_owner_public_key, msa_id).map_err(|err| { - InvalidTransaction::Custom(match err { - DispatchError::Module(sp_runtime::ModuleError { error, .. }) => { - if let Ok(decoded_err) = pallet::Error::::decode(&mut &error[..]) { - match decoded_err { - pallet::Error::::NoKeyExists => ValidityError::NoKeyExists as u8, - pallet::Error::::NotMsaOwner => ValidityError::NotMsaOwner as u8, - _ => 255u8, - } - } else { - 255u8 - } - }, - _ => 255u8, - }) - })?; + Pallet::::ensure_msa_owner(msa_owner_public_key, msa_id) + .map_err(|_| InvalidTransaction::Custom(ValidityError::InvalidMsaKey as u8))?; // - Get account address for MSA let msa_address = Pallet::::msa_id_to_eth_address(msa_id); @@ -2018,26 +1996,6 @@ impl CheckFreeExtrinsicUse { msa_balance > Zero::zero(), InvalidTransaction::Custom(ValidityError::InsufficientBalanceToWithdraw as u8) ); - - // - TODO: Phase 2 (free transaction w/fee paid from proceeds): Check that MSA balance is > fee - - let ed = T::Currency::minimum_balance(); - // Make sure either: - // - Balance to be transferred > existential deposit, OR - // - Destination account balance + ammount to be transferred > existential deposit - let amount_alone_is_insufficient = msa_balance < ed; - let dest_will_be_reaped = if amount_alone_is_insufficient { - let dest_balance = T::Currency::total_balance(receiver_account_id); - (dest_balance + msa_balance) < ed - } else { - false - }; - - ensure!( - !dest_will_be_reaped, - InvalidTransaction::Custom(ValidityError::InsufficientBalanceToFundDestination as u8) - ); - ValidTransaction::with_tag_prefix(TAG_PREFIX) .and_provides(receiver_account_id) .build() @@ -2064,24 +2022,10 @@ pub enum ValidityError { HandleNotRetired, /// Cryptographic signature verification failed MsaOwnershipInvalidSignature, - /// MSA balance is zero (paid transaction) or insufficient to cover fees (free transaction) + /// MSA balance is zero InsufficientBalanceToWithdraw, - /// MSA balance to withdraw is insufficient to fund the destination account such that it would not be reaped - InsufficientBalanceToFundDestination, - /// Fund transfer error - UnexpectedTokenTransferError, /// Origin is ineleligible for the current transaction IneligibleOrigin, - /// Proof not yet valid - ProofNotYetValid, - /// Proof has expired - ProofHasExpired, - /// Duplicate signature - SignatureAlreadySubmitted, - /// No key exists - NoKeyExists, - /// Not MSA owner - NotMsaOwner, } impl CheckFreeExtrinsicUse { @@ -2134,6 +2078,7 @@ where /// * revoke_delegation_by_delegator /// * delete_msa_public_key /// * retire_msa + /// * withdraw_tokens /// /// Validate functions for the above MUST prevent errors in the extrinsic logic to prevent spam. /// @@ -2162,12 +2107,15 @@ where msa_owner_public_key, msa_owner_proof, authorization_payload, - }) => CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( - who, - msa_owner_public_key, - msa_owner_proof, - authorization_payload, - ), + }) => { + let result = CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + who, + msa_owner_public_key, + msa_owner_proof, + authorization_payload, + ); + result + }, _ => Ok(Default::default()), } } diff --git a/pallets/msa/src/tests/msa_token_tests.rs b/pallets/msa/src/tests/msa_token_tests.rs index 376f25d04f..5145d4e196 100644 --- a/pallets/msa/src/tests/msa_token_tests.rs +++ b/pallets/msa/src/tests/msa_token_tests.rs @@ -106,7 +106,7 @@ fn it_fails_when_proof_is_expired() { &msa_signature, &payload ), - InvalidTransaction::Custom(ValidityError::ProofHasExpired as u8) + InvalidTransaction::Custom(ValidityError::MsaOwnershipInvalidSignature as u8) ); }); } @@ -133,7 +133,7 @@ fn it_fails_when_proof_is_not_yet_valid() { &msa_signature, &payload ), - InvalidTransaction::Custom(ValidityError::ProofNotYetValid as u8) + InvalidTransaction::Custom(ValidityError::MsaOwnershipInvalidSignature as u8) ); }); } @@ -175,7 +175,7 @@ fn it_fails_when_msa_key_is_not_an_msa_control_key() { &msa_signature, &payload ), - InvalidTransaction::Custom(ValidityError::NotMsaOwner as u8) + InvalidTransaction::Custom(ValidityError::InvalidMsaKey as u8) ); }) } @@ -197,7 +197,7 @@ fn it_fails_when_msa_key_does_not_control_msa_in_payload() { &msa_signature, &payload ), - InvalidTransaction::Custom(ValidityError::NoKeyExists as u8) + InvalidTransaction::Custom(ValidityError::InvalidMsaKey as u8) ); }) } @@ -265,3 +265,49 @@ fn it_succeeds_when_balance_is_sufficient() { })); }) } + +#[test] +fn it_fails_when_duplicate_signature_submitted() { + new_test_ext().execute_with(|| { + let (msa_id, owner_key_pair) = create_account(); + let (origin_key_pair, _) = sr25519::Pair::generate(); + let eth_account_id: H160 = Msa::msa_id_to_eth_address(msa_id); + let bytes: [u8; 32] = EthereumAddressMapper::to_bytes32(ð_account_id.0); + let msa_account_id = ::AccountId::from(bytes); + + let (payload, _, msa_signature) = + generate_payload(msa_id, &owner_key_pair, &origin_key_pair, None); + + let transfer_amount = 10_000_000; + + // Fund MSA + let _ = ::Currency::deposit_creating(&msa_account_id, transfer_amount); + + assert_ok!(Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature.clone(), + payload.clone() + )); + + assert_err!( + CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + &origin_key_pair.public().into(), + &owner_key_pair.public().into(), + &msa_signature, + &payload + ), + InvalidTransaction::Custom(ValidityError::MsaOwnershipInvalidSignature as u8) + ); + + assert_err!( + Msa::withdraw_tokens( + RuntimeOrigin::signed(origin_key_pair.public().into()), + owner_key_pair.public().into(), + msa_signature.clone(), + payload.clone() + ), + Error::::SignatureAlreadySubmitted + ); + }) +} From 1670a85d342a528707e95392331fa1b431d96d26 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Thu, 22 May 2025 16:36:34 -0400 Subject: [PATCH 15/19] Update pallets/msa/src/lib.rs Co-authored-by: Matthew Orris <1466844+mattheworris@users.noreply.github.com> --- pallets/msa/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index 992bd894ce..214d45f472 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -931,7 +931,7 @@ pub mod pallet { /// It also contains an expiration block number for the proof, ensuring it is valid and must be greater than the current block. /// /// # Events - /// * `pallet_balances::Event::::Transfer` - Transfer token event + /// * [`pallet_balances::Event::::Transfer`](https://docs.rs/pallet-balances/latest/pallet_balances/pallet/enum.Event.html#variant.Transfer) - Transfer token event /// /// # Errors /// From 340be0ff04538c1c1b98b6dd270fa5ccfaa6f711 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Fri, 23 May 2025 09:44:00 -0400 Subject: [PATCH 16/19] fix: revert change to enequeue_signature and update function doc --- pallets/msa/src/lib.rs | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index 214d45f472..f1d7f59683 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -1499,12 +1499,6 @@ impl Pallet { /// Raises `SignatureAlreadySubmitted` if the signature exists in the registry. /// Raises `SignatureRegistryLimitExceeded` if the oldest signature of the list has not yet expired. /// - /// Example list: - /// - `1,2 (oldest)` - /// - `2,3` - /// - `3,4` - /// - 4 (newest in pointer storage)` - /// /// # Errors /// * [`Error::ProofNotYetValid`] /// * [`Error::ProofHasExpired`] @@ -1551,6 +1545,27 @@ impl Pallet { } /// Do the actual enqueuing into the list storage and update the pointer + /// + /// The signature registry consist of two storage items: + /// - `PayloadSignatureRegistryList` - a linked list of signatures, in which each entry + /// points to the next newest signature. The list is stored as a mapping of + /// `MultiSignature` to a tuple of `(BlockNumberFor, MultiSignature)`. + /// The tuple contains the expiration block number and the next signature in the list. + /// - `PayloadSignatureRegistryPointer` - a struct containing the newest signature, + /// the oldest signature, the count of signatures, and the expiration block number of the newest signature. + /// + /// NOTE: 'newest' and 'oldest' refer to the order in which the signatures were added to the list, + /// which is not necessarily the order in which they expire. + /// + /// Example: (key [signature], value [next newest signature]) + /// - `1,2 (oldest)` + /// - `2,3` + /// - `3,4` + /// - 4 (newest in pointer storage)` + /// + // DEVELOPER NOTE: As currently implemented, the signature registry list will continue to grow until it reaches + // the maximum number of signatures, at which point it will remain at that size, only ever replacing the oldest + // signature with the newest one. This is a trade-off between storage space and performance. fn enqueue_signature( signature: &MultiSignature, signature_expires_at: BlockNumberFor, @@ -1595,7 +1610,7 @@ impl Pallet { } // Add the newest signature if we are not the first - if pointer.count != 0 && current_block.le(&pointer.newest_expires_at) { + if pointer.count != 0 { >::insert( pointer.newest, (pointer.newest_expires_at, signature.clone()), From a865495d7a29b7a7df60b1560f586dc8e6d19095 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Fri, 23 May 2025 15:19:45 -0400 Subject: [PATCH 17/19] chore: debug eth signature replay --- e2e/msa/msaTokens.test.ts | 118 ++++++++++++++---- e2e/scaffolding/extrinsicHelpers.ts | 2 +- js/ethereum-utils/src/payloads.ts | 16 +++ .../src/signature.definitions.ts | 24 ++++ js/ethereum-utils/src/signature.ts | 72 ++++++++++- pallets/msa/src/lib.rs | 33 ++++- 6 files changed, 231 insertions(+), 34 deletions(-) diff --git a/e2e/msa/msaTokens.test.ts b/e2e/msa/msaTokens.test.ts index 9d64b57ace..48987023ec 100644 --- a/e2e/msa/msaTokens.test.ts +++ b/e2e/msa/msaTokens.test.ts @@ -1,10 +1,10 @@ import '@frequency-chain/api-augment'; import assert from 'assert'; import { AuthorizedKeyData, ExtrinsicHelper } from '../scaffolding/extrinsicHelpers'; -import { ethereumAddressToKeyringPair, getUnifiedAddress, getUnifiedPublicKey } from '@frequency-chain/ethereum-utils'; +import { EcdsaSignature, createAuthorizedKeyData, ethereumAddressToKeyringPair, getUnifiedAddress, getUnifiedPublicKey, signEip712 } from '@frequency-chain/ethereum-utils'; import { getFundingSource } from '../scaffolding/funding'; import { H160 } from '@polkadot/types/interfaces'; -import { bnToU8a, hexToU8a, stringToU8a } from '@polkadot/util'; +import { bnToU8a, hexToU8a, stringToU8a, u8aToHex } from '@polkadot/util'; import { KeyringPair } from '@polkadot/keyring/types'; import { keccak256AsU8a } from '@polkadot/util-crypto'; import { @@ -13,9 +13,8 @@ import { createKeys, DOLLARS, generateAuthorizedKeyPayload, - getExistentialDeposit, + getEthereumKeyPairFromUnifiedAddress, signPayloadSr25519, - Sr25519Signature, } from '../scaffolding/helpers'; import { u64 } from '@polkadot/types'; import { Codec } from '@polkadot/types/types'; @@ -49,6 +48,23 @@ function generateMsaAddress(msaId: string | number | bigint): H160 { return ExtrinsicHelper.api.registry.createType('H160', hash.slice(-20)); } +async function generateSignedAuthorizedKeyPayload(keys: KeyringPair, payload: AuthorizedKeyData) { + const signingPayload = createAuthorizedKeyData( + payload.msaId!.toString(), + u8aToHex(payload.authorizedPublicKey), + payload.expiration + ); + const ownerSig = await signEip712( + u8aToHex(getEthereumKeyPairFromUnifiedAddress(getUnifiedAddress(keys)).secretKey), + signingPayload + ); + + return { + signingPayload, + ownerSig, + }; +} + describe('MSAs Holding Tokens', function () { const MSA_ID_1234 = 1234; // MSA ID for testing const CHECKSUMMED_ETH_ADDR_1234 = '0x65928b9a88Db189Eea76F72d86128Af834d64c32'; // Checksummed Ethereum address for MSA ID 1234 @@ -92,7 +108,6 @@ describe('MSAs Holding Tokens', function () { describe('Send tokens to MSA', function () { it('should send tokens to the MSA', async function () { - const ed = getExistentialDeposit(); let accountData = await ExtrinsicHelper.getAccountInfo(ethKeys); const initialBalance = accountData.data.free.toBigInt(); const op = ExtrinsicHelper.transferFunds( @@ -122,19 +137,19 @@ describe('MSAs Holding Tokens', function () { let secondaryKey: KeyringPair; let defaultPayload: AuthorizedKeyData; let payload: AuthorizedKeyData; - let ownerSig: Sr25519Signature; - let badSig: Sr25519Signature; + let ownerSig: EcdsaSignature; + let badSig: EcdsaSignature; let authorizedKeyData: Codec; before(async function () { // Setup an MSA with tokens - msaKeys = await createAndFundKeypair(fundingSource, 5n * CENTS); + msaKeys = await createAndFundKeypair(fundingSource, 5n * CENTS, undefined, undefined, 'ethereum'); let { target } = await ExtrinsicHelper.createMsa(msaKeys).signAndSend(); assert.notEqual(target?.data.msaId, undefined, 'MSA Id not in expected event'); msaId = target!.data.msaId; // Setup another MSA control key - otherMsaKeys = await createAndFundKeypair(fundingSource, 5n * CENTS); + otherMsaKeys = await createAndFundKeypair(fundingSource, 5n * CENTS, undefined, undefined, 'ethereum'); ({ target } = await ExtrinsicHelper.createMsa(otherMsaKeys).signAndSend()); assert.notEqual(target?.data.msaId, undefined, 'MSA Id not in expected event'); @@ -142,7 +157,7 @@ describe('MSAs Holding Tokens', function () { msaAddress = accountId; // Create unfunded keys; this extrinsic should be free - secondaryKey = createKeys(); + secondaryKey = createKeys(undefined, 'ethereum'); // Default payload making it easier to test `withdrawTokens` defaultPayload = { @@ -158,7 +173,8 @@ describe('MSAs Holding Tokens', function () { it('should fail if origin is not address contained in the payload (NotKeyOwner)', async function () { const badPayload = { ...payload, authorizedPublicKey: getUnifiedAddress(createKeys()) }; // Invalid MSA ID authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', badPayload); - ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, payload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, badPayload); await assert.rejects(op.signAndSend('current'), { name: 'RpcError', @@ -169,7 +185,8 @@ describe('MSAs Holding Tokens', function () { it('should fail if MSA owner signature is invalid (MsaOwnershipInvalidSignature)', async function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); - badSig = signPayloadSr25519(createKeys(), authorizedKeyData); // Invalid signature + // badSig = signPayloadSr25519(createKeys(), authorizedKeyData); // Invalid signature + ({ ownerSig: badSig } = await generateSignedAuthorizedKeyPayload(createKeys('badKeys', 'ethereum'), payload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, badSig, payload); await assert.rejects(op.signAndSend('current'), { name: 'RpcError', @@ -184,7 +201,8 @@ describe('MSAs Holding Tokens', function () { expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber(), }); authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, newPayload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { name: 'RpcError', @@ -200,7 +218,8 @@ describe('MSAs Holding Tokens', function () { expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber() + maxMortality + 999, }); authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, newPayload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { name: 'RpcError', @@ -216,7 +235,8 @@ describe('MSAs Holding Tokens', function () { authorizedPublicKey: getUnifiedPublicKey(otherMsaKeys), }); authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, newPayload)); const op = ExtrinsicHelper.withdrawTokens(otherMsaKeys, msaKeys, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { name: 'RpcError', @@ -231,7 +251,8 @@ describe('MSAs Holding Tokens', function () { msaId: new u64(ExtrinsicHelper.api.registry, 9999), }); authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, newPayload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { name: 'RpcError', @@ -241,13 +262,14 @@ describe('MSAs Holding Tokens', function () { }); it('should fail if payload signer is not an MSA control key (InvalidMsaKey)', async function () { - const badSigner = createKeys(); + const badSigner = createKeys(undefined, 'ethereum'); const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, msaId: new u64(ExtrinsicHelper.api.registry, 9999), }); authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - ownerSig = signPayloadSr25519(badSigner, authorizedKeyData); + // ownerSig = signPayloadSr25519(badSigner, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(badSigner, newPayload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, badSigner, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { name: 'RpcError', @@ -258,7 +280,8 @@ describe('MSAs Holding Tokens', function () { it('should fail if MSA does not have a balance', async function () { authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); - ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, payload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); await assert.rejects(op.signAndSend('current'), { name: 'RpcError', @@ -279,7 +302,8 @@ describe('MSAs Holding Tokens', function () { ); await assert.doesNotReject(op1.signAndSend(), 'MSA funding failed'); authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); - ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, payload)); const op2 = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); await assert.doesNotReject(op2.signAndSend('current'), 'token transfer transaction should have succeeded'); // Destination account should have had balance increased @@ -293,11 +317,20 @@ describe('MSAs Holding Tokens', function () { ); }); - it('should fail for duplicate signature submission (MsaOwnershipInvalidSignature)', async function () { + it('should fail for duplicate Sr25519 signature submission (MsaOwnershipInvalidSignature)', async function () { + // Create a new MSA with Sr25519 keys + const srMsaKeys = await createAndFundKeypair(fundingSource, 5n * CENTS); + const { target: msaEvent } = await ExtrinsicHelper.createMsa(srMsaKeys).signAndSend(); + assert.notEqual(msaEvent?.data.msaId, undefined, 'MSA Id not in expected event'); + const msaId = msaEvent!.data.msaId; + const { accountId } = await ExtrinsicHelper.apiPromise.call.msaRuntimeApi.getEthereumAddressForMsaId(msaId); + const msaAddress = accountId; + // Due to the fact that we're testing this free extrinsic with unfunded accounts (ie, with no nonce), // we'll use a unique keypair for this test so that it doesn't matter whether other tests using // the same keypair ended up funding the account & therefore initializing a nonce. const keys = createKeys(); + payload.msaId = msaId; payload.authorizedPublicKey = getUnifiedPublicKey(keys); const op1 = ExtrinsicHelper.transferFunds( @@ -307,20 +340,53 @@ describe('MSAs Holding Tokens', function () { ); await assert.doesNotReject(op1.signAndSend(), 'MSA funding failed'); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); - ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); - let op2 = ExtrinsicHelper.withdrawTokens(keys, msaKeys, ownerSig, payload); + const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, authorizedPublicKey: getUnifiedPublicKey(keys), msaId }); + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); + const ownerSig = signPayloadSr25519(srMsaKeys, authorizedKeyData); + let op2 = ExtrinsicHelper.withdrawTokens(keys, srMsaKeys, ownerSig, newPayload); await assert.doesNotReject(op2.signAndSend('current'), 'token withdrawal should have succeeded'); // Re-fund MSA so we don't fail for that await assert.doesNotReject(op1.signAndSend(), 'MSA re-funding failed'); - - op2 = ExtrinsicHelper.withdrawTokens(keys, msaKeys, ownerSig, payload); +console.log(`Submitting duplicate signagture: ${JSON.stringify(ownerSig)}`); + op2 = ExtrinsicHelper.withdrawTokens(keys, srMsaKeys, ownerSig, newPayload); await assert.rejects(op2.signAndSend(0), { name: 'RpcError', code: 1010, data: 'Custom error: 8', // MsaOwnershipInvalidSignature, }); }); + + it('should fail for duplicate signature submission (MsaOwnershipInvalidSignature)', async function () { + // Due to the fact that we're testing this free extrinsic with unfunded accounts (ie, with no nonce), + // we'll use a unique keypair for this test so that it doesn't matter whether other tests using + // the same keypair ended up funding the account & therefore initializing a nonce. + const keys = createKeys(undefined, 'ethereum'); + payload.authorizedPublicKey = getUnifiedPublicKey(keys); + + const op1 = ExtrinsicHelper.transferFunds( + fundingSource, + ethereumAddressToKeyringPair(msaAddress), + TRANSFER_AMOUNT + ); + await assert.doesNotReject(op1.signAndSend(), 'MSA funding failed'); + + authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); + // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); + ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, payload)); + let op2 = ExtrinsicHelper.withdrawTokens(keys, msaKeys, ownerSig, payload); + await assert.doesNotReject(op2.signAndSend('current'), 'token withdrawal should have succeeded'); + + // Re-fund MSA so we don't fail for that + await assert.doesNotReject(op1.signAndSend(), 'MSA re-funding failed'); +console.log(`Submitting duplicate signature: ${JSON.stringify(ownerSig)}`); + op2 = ExtrinsicHelper.withdrawTokens(keys, msaKeys, ownerSig, payload); + await assert.doesNotReject(op2.signAndSend('current'), 'token withdrawal should have succeeded'); + // await assert.rejects(op2.signAndSend('current'), { + // name: 'RpcError', + // code: 1010, + // data: 'Custom error: 8', // MsaOwnershipInvalidSignature, + // }); + }); }); }); diff --git a/e2e/scaffolding/extrinsicHelpers.ts b/e2e/scaffolding/extrinsicHelpers.ts index c91f98cd83..7d9af61278 100644 --- a/e2e/scaffolding/extrinsicHelpers.ts +++ b/e2e/scaffolding/extrinsicHelpers.ts @@ -42,7 +42,7 @@ export interface AddKeyData { } export interface AuthorizedKeyData { msaId: u64; - expiration?: AnyNumber; + expiration?: number | any; authorizedPublicKey: KeyringPair['publicKey']; } export interface AddProviderPayload { diff --git a/js/ethereum-utils/src/payloads.ts b/js/ethereum-utils/src/payloads.ts index 8408074888..6f36a63767 100644 --- a/js/ethereum-utils/src/payloads.ts +++ b/js/ethereum-utils/src/payloads.ts @@ -115,6 +115,17 @@ export interface AddKeyData { newPublicKey: HexString; } +export interface AuthorizedKeyData { + // type discriminator + type: 'AuthorizedKeyData'; + // uint64 type MessageSourceId + msaId: string; + // uint32 type payload expiration block number + expiration: number; + // hex encoded public key to be signed + authorizedPublicKey: HexString; +} + export interface AddProvider { // type discriminator type: 'AddProvider'; @@ -133,11 +144,16 @@ export type SupportedPayload = | PasskeyPublicKey | ClaimHandlePayload | AddKeyData + | AuthorizedKeyData | AddProvider; +export type NormalizedSupportedPayload = Omit; + export interface EipDomainPayload { name: string; version: string; chainId: HexString; verifyingContract: HexString; } + +export type SupportedPayloadTypes = SupportedPayload['type']; diff --git a/js/ethereum-utils/src/signature.definitions.ts b/js/ethereum-utils/src/signature.definitions.ts index f17f005f42..b94fe5859e 100644 --- a/js/ethereum-utils/src/signature.definitions.ts +++ b/js/ethereum-utils/src/signature.definitions.ts @@ -63,6 +63,23 @@ export const ADD_KEY_DATA_DEFINITION = { ], }; +export const AUTHORIZED_KEY_DATA_DEFINITION = { + AuthorizedKeyData: [ + { + name: 'msaId', + type: 'uint64', + }, + { + name: 'expiration', + type: 'uint32', + }, + { + name: 'authorizedPublicKey', + type: 'address', + }, + ], +}; + export const CLAIM_HANDLE_PAYLOAD_DEFINITION = { ClaimHandlePayload: [ { @@ -156,3 +173,10 @@ export const ITEMIZED_SIGNATURE_PAYLOAD_DEFINITION_V2 = { { name: 'index', type: 'uint16' }, ], }; + +const PAYLOAD_DEFINITIONS = [ADD_PROVIDER_DEFINITION, ADD_KEY_DATA_DEFINITION, AUTHORIZED_KEY_DATA_DEFINITION, CLAIM_HANDLE_PAYLOAD_DEFINITION, + PASSKEY_PUBLIC_KEY_DEFINITION, PAGINATED_DELETE_SIGNATURE_PAYLOAD_DEFINITION_V2, PAGINATED_UPSERT_SIGNATURE_PAYLOAD_DEFINITION_V2, + ITEMIZED_SIGNATURE_PAYLOAD_DEFINITION_V2, +]; + +export type SupportedPayloadDefinitions = typeof PAYLOAD_DEFINITIONS[number]; diff --git a/js/ethereum-utils/src/signature.ts b/js/ethereum-utils/src/signature.ts index ed84d86212..90b4fe4dfc 100644 --- a/js/ethereum-utils/src/signature.ts +++ b/js/ethereum-utils/src/signature.ts @@ -1,5 +1,6 @@ import { AddKeyData, + AuthorizedKeyData, AddProvider, ChainType, ClaimHandlePayload, @@ -14,14 +15,17 @@ import { DeleteItemizedAction, ItemizedAction, EipDomainPayload, + NormalizedSupportedPayload, + SupportedPayloadTypes, } from './payloads'; import { assert, isHexString, isValidUint16, isValidUint32, isValidUint64String } from './utils'; import { reverseUnifiedAddressToEthereumAddress } from './address'; -import { ethers, TypedDataField } from 'ethers'; +import { ethers } from 'ethers'; import { u8aToHex } from '@polkadot/util'; import { ADD_KEY_DATA_DEFINITION, ADD_PROVIDER_DEFINITION, + AUTHORIZED_KEY_DATA_DEFINITION, CLAIM_HANDLE_PAYLOAD_DEFINITION, EIP712_DOMAIN_DEFAULT, EIP712_DOMAIN_DEFINITION, @@ -29,6 +33,7 @@ import { PAGINATED_DELETE_SIGNATURE_PAYLOAD_DEFINITION_V2, PAGINATED_UPSERT_SIGNATURE_PAYLOAD_DEFINITION_V2, PASSKEY_PUBLIC_KEY_DEFINITION, + SupportedPayloadDefinitions, } from './signature.definitions'; /** @@ -72,8 +77,8 @@ export function verifyEip712Signature( return recoveredAddress.toLowerCase() === ethereumAddress.toLowerCase(); } -function normalizePayload(payload: SupportedPayload): Record { - const clonedPayload = Object.assign({}, payload); +function normalizePayload(payload: SupportedPayload): NormalizedSupportedPayload { + const clonedPayload: typeof payload = Object.assign({}, payload); switch (clonedPayload.type) { case 'PaginatedUpsertSignaturePayloadV2': case 'PaginatedDeleteSignaturePayloadV2': @@ -91,6 +96,15 @@ function normalizePayload(payload: SupportedPayload): Record { clonedPayload.newPublicKey = clonedPayload.newPublicKey.toLowerCase() as HexString; break; + case 'AuthorizedKeyData': + // convert to 20 bytes ethereum address for signature + if (clonedPayload.authorizedPublicKey.length !== 42) { + clonedPayload.authorizedPublicKey = reverseUnifiedAddressToEthereumAddress((payload as AuthorizedKeyData).authorizedPublicKey); + } + clonedPayload.authorizedPublicKey = clonedPayload.authorizedPublicKey.toLowerCase() as HexString; + break; + + default: throw new Error(`Unsupported payload type: ${JSON.stringify(payload)}`); } @@ -101,14 +115,15 @@ function normalizePayload(payload: SupportedPayload): Record { return payloadWithoutType; } -function getTypesFor(payloadType: string): Record { - const PAYLOAD_TYPE_DEFINITIONS: Record> = { +function getTypesFor(payloadType: string): SupportedPayloadDefinitions { + const PAYLOAD_TYPE_DEFINITIONS: Record = { PaginatedUpsertSignaturePayloadV2: PAGINATED_UPSERT_SIGNATURE_PAYLOAD_DEFINITION_V2, PaginatedDeleteSignaturePayloadV2: PAGINATED_DELETE_SIGNATURE_PAYLOAD_DEFINITION_V2, ItemizedSignaturePayloadV2: ITEMIZED_SIGNATURE_PAYLOAD_DEFINITION_V2, PasskeyPublicKey: PASSKEY_PUBLIC_KEY_DEFINITION, ClaimHandlePayload: CLAIM_HANDLE_PAYLOAD_DEFINITION, AddKeyData: ADD_KEY_DATA_DEFINITION, + AuthorizedKeyData: AUTHORIZED_KEY_DATA_DEFINITION, AddProvider: ADD_PROVIDER_DEFINITION, }; @@ -147,6 +162,32 @@ export function createAddKeyData( }; } +/** + * Build an AuthorizedKeyData payload for signature. + * + * @param msaId MSA ID (uint64) to add the key + * @param authorizedPublicKey 32 bytes public key to authorize in hex or Uint8Array + * @param expirationBlock Block number after which this payload is invalid + */ +export function createAuthorizedKeyData( + msaId: string | bigint, + newPublicKey: HexString | Uint8Array, + expirationBlock: number +): AuthorizedKeyData { + const parsedMsaId: string = typeof msaId === 'string' ? msaId : `${msaId}`; + const parsedNewPublicKey: HexString = typeof newPublicKey === 'object' ? u8aToHex(newPublicKey) : newPublicKey; + + assert(isValidUint64String(parsedMsaId), 'msaId should be a valid uint64'); + assert(isValidUint32(expirationBlock), 'expiration should be a valid uint32'); + assert(isHexString(parsedNewPublicKey), 'newPublicKey should be valid hex'); + return { + type: 'AuthorizedKeyData', + msaId: parsedMsaId, + expiration: expirationBlock, + authorizedPublicKey: parsedNewPublicKey, + }; +} + /** * Build an AddProvider payload for signature. * @@ -326,6 +367,25 @@ export function getEip712BrowserRequestAddKeyData( return createEip712Payload(ADD_KEY_DATA_DEFINITION, message.type, domain, normalized); } +/** + * Returns the EIP-712 browser request for a AuthorizedKeyData for signing. + * + * @param msaId MSA ID (uint64) to add the key + * @param authorizedPublicKey 32 bytes public key to add in hex or Uint8Array + * @param expirationBlock Block number after which this payload is invalid + * @param domain + */ +export function getEip712BrowserRequestAuthorizedKeyData( + msaId: string | bigint, + authorizedPublicKey: HexString | Uint8Array, + expirationBlock: number, + domain: EipDomainPayload = EIP712_DOMAIN_DEFAULT +): unknown { + const message = createAuthorizedKeyData(msaId, authorizedPublicKey, expirationBlock); + const normalized = normalizePayload(message); + return createEip712Payload(AUTHORIZED_KEY_DATA_DEFINITION, message.type, domain, normalized); +} + /** * Returns the EIP-712 browser request for a AddProvider for signing. * @@ -442,7 +502,7 @@ export function getEip712BrowserRequestPasskeyPublicKey( return createEip712Payload(PASSKEY_PUBLIC_KEY_DEFINITION, message.type, domain, normalized); } -function createEip712Payload(typeDefinition: any, primaryType: any, domain: EipDomainPayload, message: any): unknown { +function createEip712Payload(typeDefinition: SupportedPayloadDefinitions, primaryType: SupportedPayloadTypes, domain: EipDomainPayload, message: NormalizedSupportedPayload): unknown { return { types: { ...EIP712_DOMAIN_DEFINITION, diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index f1d7f59683..290fb219fe 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -1514,6 +1514,24 @@ impl Pallet { Self::enqueue_signature(signature, signature_expires_at, current_block) } + fn display_signature( + signature: &MultiSignature, + ) { + use sp_core::hexdisplay; + match signature { + MultiSignature::Ecdsa(signature) => { + log::warn!("{:?}", hexdisplay::HexDisplay::from(&&signature.0[..])); + }, + MultiSignature::Sr25519(signature) => { + log::warn!("{:?}", hexdisplay::HexDisplay::from(&&signature.0[..])); + }, + MultiSignature::Ed25519(signature) => { + log::warn!("{:?}", hexdisplay::HexDisplay::from(&&signature.0[..])); + }, + }; + } + + /// Check that mortality_block is within bounds. /// Raises `SignatureAlreadySubmitted` if the signature exists in the registry. /// @@ -1537,7 +1555,11 @@ impl Pallet { !>::contains_key(signature), Error::::SignatureAlreadySubmitted ); - if let Some(signature_pointer) = PayloadSignatureRegistryPointer::::get() { + log::warn!("Checking for duplicate signature in pointer:"); + if let Ok(signature_pointer) = PayloadSignatureRegistryPointer::::try_get() { + log::warn!("Checking for duplicate signature (incoming, pointer):"); + Self::display_signature(signature); + Self::display_signature(&signature_pointer.newest); ensure!(signature_pointer.newest != *signature, Error::::SignatureAlreadySubmitted); } @@ -1571,6 +1593,8 @@ impl Pallet { signature_expires_at: BlockNumberFor, current_block: BlockNumberFor, ) -> DispatchResult { + log::warn!("Enqueuing signature:"); + Self::display_signature(signature); // Get the current pointer, or if this is the initialization, generate an empty pointer let pointer = PayloadSignatureRegistryPointer::::get().unwrap_or(SignatureRegistryPointer { @@ -2084,6 +2108,12 @@ where info: &DispatchInfoOf, len: usize, ) -> Result { + match call.is_sub_type() { + Some(Call::withdraw_tokens { .. }) => { + log::warn!("CheckFreeExtrinsicUse::pre_dispatch: withdraw_tokens"); + }, + _ => {} + } self.validate(who, call, info, len).map(|_| ()) } @@ -2123,6 +2153,7 @@ where msa_owner_proof, authorization_payload, }) => { + log::warn!("Validating MSA token withdrawal"); let result = CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( who, msa_owner_public_key, From c857b30066ab8895c6bd8334928d1d7a82e76343 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Fri, 23 May 2025 16:12:58 -0400 Subject: [PATCH 18/19] fix: formatting --- e2e/msa/msaTokens.test.ts | 114 +++++------------- .../src/signature.definitions.ts | 12 +- js/ethereum-utils/src/signature.ts | 12 +- pallets/msa/src/lib.rs | 48 ++------ 4 files changed, 53 insertions(+), 133 deletions(-) diff --git a/e2e/msa/msaTokens.test.ts b/e2e/msa/msaTokens.test.ts index 48987023ec..307f5f74fe 100644 --- a/e2e/msa/msaTokens.test.ts +++ b/e2e/msa/msaTokens.test.ts @@ -1,7 +1,14 @@ import '@frequency-chain/api-augment'; import assert from 'assert'; import { AuthorizedKeyData, ExtrinsicHelper } from '../scaffolding/extrinsicHelpers'; -import { EcdsaSignature, createAuthorizedKeyData, ethereumAddressToKeyringPair, getUnifiedAddress, getUnifiedPublicKey, signEip712 } from '@frequency-chain/ethereum-utils'; +import { + EcdsaSignature, + createAuthorizedKeyData, + ethereumAddressToKeyringPair, + getUnifiedAddress, + getUnifiedPublicKey, + signEip712, +} from '@frequency-chain/ethereum-utils'; import { getFundingSource } from '../scaffolding/funding'; import { H160 } from '@polkadot/types/interfaces'; import { bnToU8a, hexToU8a, stringToU8a, u8aToHex } from '@polkadot/util'; @@ -14,7 +21,6 @@ import { DOLLARS, generateAuthorizedKeyPayload, getEthereumKeyPairFromUnifiedAddress, - signPayloadSr25519, } from '../scaffolding/helpers'; import { u64 } from '@polkadot/types'; import { Codec } from '@polkadot/types/types'; @@ -49,20 +55,20 @@ function generateMsaAddress(msaId: string | number | bigint): H160 { } async function generateSignedAuthorizedKeyPayload(keys: KeyringPair, payload: AuthorizedKeyData) { - const signingPayload = createAuthorizedKeyData( - payload.msaId!.toString(), - u8aToHex(payload.authorizedPublicKey), - payload.expiration - ); - const ownerSig = await signEip712( - u8aToHex(getEthereumKeyPairFromUnifiedAddress(getUnifiedAddress(keys)).secretKey), - signingPayload - ); - - return { - signingPayload, - ownerSig, - }; + const signingPayload = createAuthorizedKeyData( + payload.msaId!.toString(), + u8aToHex(payload.authorizedPublicKey), + payload.expiration + ); + const ownerSig = await signEip712( + u8aToHex(getEthereumKeyPairFromUnifiedAddress(getUnifiedAddress(keys)).secretKey), + signingPayload + ); + + return { + signingPayload, + ownerSig, + }; } describe('MSAs Holding Tokens', function () { @@ -139,7 +145,6 @@ describe('MSAs Holding Tokens', function () { let payload: AuthorizedKeyData; let ownerSig: EcdsaSignature; let badSig: EcdsaSignature; - let authorizedKeyData: Codec; before(async function () { // Setup an MSA with tokens @@ -172,8 +177,6 @@ describe('MSAs Holding Tokens', function () { it('should fail if origin is not address contained in the payload (NotKeyOwner)', async function () { const badPayload = { ...payload, authorizedPublicKey: getUnifiedAddress(createKeys()) }; // Invalid MSA ID - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', badPayload); - // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, payload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, badPayload); await assert.rejects(op.signAndSend('current'), { @@ -184,8 +187,6 @@ describe('MSAs Holding Tokens', function () { }); it('should fail if MSA owner signature is invalid (MsaOwnershipInvalidSignature)', async function () { - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); - // badSig = signPayloadSr25519(createKeys(), authorizedKeyData); // Invalid signature ({ ownerSig: badSig } = await generateSignedAuthorizedKeyPayload(createKeys('badKeys', 'ethereum'), payload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, badSig, payload); await assert.rejects(op.signAndSend('current'), { @@ -200,8 +201,6 @@ describe('MSAs Holding Tokens', function () { ...defaultPayload, expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber(), }); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, newPayload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { @@ -217,8 +216,6 @@ describe('MSAs Holding Tokens', function () { ...defaultPayload, expiration: (await ExtrinsicHelper.getLastBlock()).block.header.number.toNumber() + maxMortality + 999, }); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, newPayload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { @@ -229,13 +226,10 @@ describe('MSAs Holding Tokens', function () { }); it('should fail if origin is an MSA control key (IneligibleOrigin)', async function () { - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, authorizedPublicKey: getUnifiedPublicKey(otherMsaKeys), }); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, newPayload)); const op = ExtrinsicHelper.withdrawTokens(otherMsaKeys, msaKeys, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { @@ -250,8 +244,6 @@ describe('MSAs Holding Tokens', function () { ...defaultPayload, msaId: new u64(ExtrinsicHelper.api.registry, 9999), }); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, newPayload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { @@ -267,8 +259,6 @@ describe('MSAs Holding Tokens', function () { ...defaultPayload, msaId: new u64(ExtrinsicHelper.api.registry, 9999), }); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - // ownerSig = signPayloadSr25519(badSigner, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(badSigner, newPayload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, badSigner, ownerSig, newPayload); await assert.rejects(op.signAndSend('current'), { @@ -279,8 +269,6 @@ describe('MSAs Holding Tokens', function () { }); it('should fail if MSA does not have a balance', async function () { - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); - // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, payload)); const op = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); await assert.rejects(op.signAndSend('current'), { @@ -301,8 +289,6 @@ describe('MSAs Holding Tokens', function () { TRANSFER_AMOUNT ); await assert.doesNotReject(op1.signAndSend(), 'MSA funding failed'); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); - // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, payload)); const op2 = ExtrinsicHelper.withdrawTokens(secondaryKey, msaKeys, ownerSig, payload); await assert.doesNotReject(op2.signAndSend('current'), 'token transfer transaction should have succeeded'); @@ -317,51 +303,11 @@ describe('MSAs Holding Tokens', function () { ); }); - it('should fail for duplicate Sr25519 signature submission (MsaOwnershipInvalidSignature)', async function () { - // Create a new MSA with Sr25519 keys - const srMsaKeys = await createAndFundKeypair(fundingSource, 5n * CENTS); - const { target: msaEvent } = await ExtrinsicHelper.createMsa(srMsaKeys).signAndSend(); - assert.notEqual(msaEvent?.data.msaId, undefined, 'MSA Id not in expected event'); - const msaId = msaEvent!.data.msaId; - const { accountId } = await ExtrinsicHelper.apiPromise.call.msaRuntimeApi.getEthereumAddressForMsaId(msaId); - const msaAddress = accountId; - - // Due to the fact that we're testing this free extrinsic with unfunded accounts (ie, with no nonce), - // we'll use a unique keypair for this test so that it doesn't matter whether other tests using - // the same keypair ended up funding the account & therefore initializing a nonce. - const keys = createKeys(); - payload.msaId = msaId; - payload.authorizedPublicKey = getUnifiedPublicKey(keys); - - const op1 = ExtrinsicHelper.transferFunds( - fundingSource, - ethereumAddressToKeyringPair(msaAddress), - TRANSFER_AMOUNT - ); - await assert.doesNotReject(op1.signAndSend(), 'MSA funding failed'); - - const newPayload = await generateAuthorizedKeyPayload({ ...defaultPayload, authorizedPublicKey: getUnifiedPublicKey(keys), msaId }); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', newPayload); - const ownerSig = signPayloadSr25519(srMsaKeys, authorizedKeyData); - let op2 = ExtrinsicHelper.withdrawTokens(keys, srMsaKeys, ownerSig, newPayload); - await assert.doesNotReject(op2.signAndSend('current'), 'token withdrawal should have succeeded'); - - // Re-fund MSA so we don't fail for that - await assert.doesNotReject(op1.signAndSend(), 'MSA re-funding failed'); -console.log(`Submitting duplicate signagture: ${JSON.stringify(ownerSig)}`); - op2 = ExtrinsicHelper.withdrawTokens(keys, srMsaKeys, ownerSig, newPayload); - await assert.rejects(op2.signAndSend(0), { - name: 'RpcError', - code: 1010, - data: 'Custom error: 8', // MsaOwnershipInvalidSignature, - }); - }); - it('should fail for duplicate signature submission (MsaOwnershipInvalidSignature)', async function () { // Due to the fact that we're testing this free extrinsic with unfunded accounts (ie, with no nonce), // we'll use a unique keypair for this test so that it doesn't matter whether other tests using // the same keypair ended up funding the account & therefore initializing a nonce. - const keys = createKeys(undefined, 'ethereum'); + const keys = await createAndFundKeypair(fundingSource, 5n * CENTS, undefined, undefined, 'ethereum'); payload.authorizedPublicKey = getUnifiedPublicKey(keys); const op1 = ExtrinsicHelper.transferFunds( @@ -371,22 +317,18 @@ console.log(`Submitting duplicate signagture: ${JSON.stringify(ownerSig)}`); ); await assert.doesNotReject(op1.signAndSend(), 'MSA funding failed'); - authorizedKeyData = ExtrinsicHelper.api.registry.createType('PalletMsaAuthorizedKeyData', payload); - // ownerSig = signPayloadSr25519(msaKeys, authorizedKeyData); ({ ownerSig } = await generateSignedAuthorizedKeyPayload(msaKeys, payload)); let op2 = ExtrinsicHelper.withdrawTokens(keys, msaKeys, ownerSig, payload); await assert.doesNotReject(op2.signAndSend('current'), 'token withdrawal should have succeeded'); // Re-fund MSA so we don't fail for that await assert.doesNotReject(op1.signAndSend(), 'MSA re-funding failed'); -console.log(`Submitting duplicate signature: ${JSON.stringify(ownerSig)}`); op2 = ExtrinsicHelper.withdrawTokens(keys, msaKeys, ownerSig, payload); - await assert.doesNotReject(op2.signAndSend('current'), 'token withdrawal should have succeeded'); - // await assert.rejects(op2.signAndSend('current'), { - // name: 'RpcError', - // code: 1010, - // data: 'Custom error: 8', // MsaOwnershipInvalidSignature, - // }); + await assert.rejects(op2.signAndSend('current'), { + name: 'RpcError', + code: 1010, + data: 'Custom error: 8', // MsaOwnershipInvalidSignature, + }); }); }); }); diff --git a/js/ethereum-utils/src/signature.definitions.ts b/js/ethereum-utils/src/signature.definitions.ts index b94fe5859e..a24ce13d11 100644 --- a/js/ethereum-utils/src/signature.definitions.ts +++ b/js/ethereum-utils/src/signature.definitions.ts @@ -174,9 +174,15 @@ export const ITEMIZED_SIGNATURE_PAYLOAD_DEFINITION_V2 = { ], }; -const PAYLOAD_DEFINITIONS = [ADD_PROVIDER_DEFINITION, ADD_KEY_DATA_DEFINITION, AUTHORIZED_KEY_DATA_DEFINITION, CLAIM_HANDLE_PAYLOAD_DEFINITION, - PASSKEY_PUBLIC_KEY_DEFINITION, PAGINATED_DELETE_SIGNATURE_PAYLOAD_DEFINITION_V2, PAGINATED_UPSERT_SIGNATURE_PAYLOAD_DEFINITION_V2, +const PAYLOAD_DEFINITIONS = [ + ADD_PROVIDER_DEFINITION, + ADD_KEY_DATA_DEFINITION, + AUTHORIZED_KEY_DATA_DEFINITION, + CLAIM_HANDLE_PAYLOAD_DEFINITION, + PASSKEY_PUBLIC_KEY_DEFINITION, + PAGINATED_DELETE_SIGNATURE_PAYLOAD_DEFINITION_V2, + PAGINATED_UPSERT_SIGNATURE_PAYLOAD_DEFINITION_V2, ITEMIZED_SIGNATURE_PAYLOAD_DEFINITION_V2, ]; -export type SupportedPayloadDefinitions = typeof PAYLOAD_DEFINITIONS[number]; +export type SupportedPayloadDefinitions = (typeof PAYLOAD_DEFINITIONS)[number]; diff --git a/js/ethereum-utils/src/signature.ts b/js/ethereum-utils/src/signature.ts index 90b4fe4dfc..360e0f87df 100644 --- a/js/ethereum-utils/src/signature.ts +++ b/js/ethereum-utils/src/signature.ts @@ -99,12 +99,13 @@ function normalizePayload(payload: SupportedPayload): NormalizedSupportedPayload case 'AuthorizedKeyData': // convert to 20 bytes ethereum address for signature if (clonedPayload.authorizedPublicKey.length !== 42) { - clonedPayload.authorizedPublicKey = reverseUnifiedAddressToEthereumAddress((payload as AuthorizedKeyData).authorizedPublicKey); + clonedPayload.authorizedPublicKey = reverseUnifiedAddressToEthereumAddress( + (payload as AuthorizedKeyData).authorizedPublicKey + ); } clonedPayload.authorizedPublicKey = clonedPayload.authorizedPublicKey.toLowerCase() as HexString; break; - default: throw new Error(`Unsupported payload type: ${JSON.stringify(payload)}`); } @@ -502,7 +503,12 @@ export function getEip712BrowserRequestPasskeyPublicKey( return createEip712Payload(PASSKEY_PUBLIC_KEY_DEFINITION, message.type, domain, normalized); } -function createEip712Payload(typeDefinition: SupportedPayloadDefinitions, primaryType: SupportedPayloadTypes, domain: EipDomainPayload, message: NormalizedSupportedPayload): unknown { +function createEip712Payload( + typeDefinition: SupportedPayloadDefinitions, + primaryType: SupportedPayloadTypes, + domain: EipDomainPayload, + message: NormalizedSupportedPayload +): unknown { return { types: { ...EIP712_DOMAIN_DEFINITION, diff --git a/pallets/msa/src/lib.rs b/pallets/msa/src/lib.rs index 290fb219fe..19372d97a4 100644 --- a/pallets/msa/src/lib.rs +++ b/pallets/msa/src/lib.rs @@ -1514,24 +1514,6 @@ impl Pallet { Self::enqueue_signature(signature, signature_expires_at, current_block) } - fn display_signature( - signature: &MultiSignature, - ) { - use sp_core::hexdisplay; - match signature { - MultiSignature::Ecdsa(signature) => { - log::warn!("{:?}", hexdisplay::HexDisplay::from(&&signature.0[..])); - }, - MultiSignature::Sr25519(signature) => { - log::warn!("{:?}", hexdisplay::HexDisplay::from(&&signature.0[..])); - }, - MultiSignature::Ed25519(signature) => { - log::warn!("{:?}", hexdisplay::HexDisplay::from(&&signature.0[..])); - }, - }; - } - - /// Check that mortality_block is within bounds. /// Raises `SignatureAlreadySubmitted` if the signature exists in the registry. /// @@ -1555,11 +1537,7 @@ impl Pallet { !>::contains_key(signature), Error::::SignatureAlreadySubmitted ); - log::warn!("Checking for duplicate signature in pointer:"); - if let Ok(signature_pointer) = PayloadSignatureRegistryPointer::::try_get() { - log::warn!("Checking for duplicate signature (incoming, pointer):"); - Self::display_signature(signature); - Self::display_signature(&signature_pointer.newest); + if let Some(signature_pointer) = PayloadSignatureRegistryPointer::::get() { ensure!(signature_pointer.newest != *signature, Error::::SignatureAlreadySubmitted); } @@ -1593,8 +1571,6 @@ impl Pallet { signature_expires_at: BlockNumberFor, current_block: BlockNumberFor, ) -> DispatchResult { - log::warn!("Enqueuing signature:"); - Self::display_signature(signature); // Get the current pointer, or if this is the initialization, generate an empty pointer let pointer = PayloadSignatureRegistryPointer::::get().unwrap_or(SignatureRegistryPointer { @@ -2108,12 +2084,6 @@ where info: &DispatchInfoOf, len: usize, ) -> Result { - match call.is_sub_type() { - Some(Call::withdraw_tokens { .. }) => { - log::warn!("CheckFreeExtrinsicUse::pre_dispatch: withdraw_tokens"); - }, - _ => {} - } self.validate(who, call, info, len).map(|_| ()) } @@ -2152,16 +2122,12 @@ where msa_owner_public_key, msa_owner_proof, authorization_payload, - }) => { - log::warn!("Validating MSA token withdrawal"); - let result = CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( - who, - msa_owner_public_key, - msa_owner_proof, - authorization_payload, - ); - result - }, + }) => CheckFreeExtrinsicUse::::validate_msa_token_withdrawal( + who, + msa_owner_public_key, + msa_owner_proof, + authorization_payload, + ), _ => Ok(Default::default()), } } From 0a281769b4afa7e14c6f691d4ea745f597efa163 Mon Sep 17 00:00:00 2001 From: Joe Caputo Date: Fri, 23 May 2025 16:28:28 -0400 Subject: [PATCH 19/19] fix: comment --- e2e/msa/msaTokens.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/e2e/msa/msaTokens.test.ts b/e2e/msa/msaTokens.test.ts index 307f5f74fe..728d3f4269 100644 --- a/e2e/msa/msaTokens.test.ts +++ b/e2e/msa/msaTokens.test.ts @@ -304,9 +304,10 @@ describe('MSAs Holding Tokens', function () { }); it('should fail for duplicate signature submission (MsaOwnershipInvalidSignature)', async function () { - // Due to the fact that we're testing this free extrinsic with unfunded accounts (ie, with no nonce), - // we'll use a unique keypair for this test so that it doesn't matter whether other tests using - // the same keypair ended up funding the account & therefore initializing a nonce. + // In order to test this, we need to create a new keypair and fund it, because otherwise the nonce will + // be the same for both transactions (and, because we're using Edcs signatures, the signature will be the same). + // Not sure exactly what happens in this case, but it seems to be that the second transaction is siliently dropped + // by the node, but the status call back in polkadot.js still resolves (ie, gets 'isInBlock' or 'isFinalized') const keys = await createAndFundKeypair(fundingSource, 5n * CENTS, undefined, undefined, 'ethereum'); payload.authorizedPublicKey = getUnifiedPublicKey(keys);