Skip to content

multisign: decouple tx_list borrow lifetime from T's Transaction<'a> lifetime #306

Description

@ckeshava

Problem

transaction::multisign has a signature that conflates two lifetimes that
should be independent:

pub fn multisign<'a, T, F>(transaction: &mut T, tx_list: &'a Vec<T>) -> XRPLHelperResult<()>
where
    F: IntoEnumIterator + Serialize + Debug + PartialEq + 'a,
    T: Transaction<'a, F>,

'a is used both as:

  1. The borrow lifetime of tx_list.
  2. The data lifetime of T: Transaction<'a, F>.

Because they share one parameter, the compiler must pick a single 'a that
satisfies both. Once a caller's T lifetime is inferred as 'static — which
happens routinely when the transaction is built from string literals
(XRPAmount::from("20000000") → Cow::Borrowed<'static, str>) and used in
an async block that pushes the inferred lifetime to 'static — the borrow
of tx_list is also required to be 'static.

Observed symptom

Callers cannot pass a &local_vec to multisign from an async test that
also performs autofill: they have to Box::leak the Vec to obtain a
&'static Vec<T>. Concrete instance introduced by PR #298 (commit 6031ad2):

tests/transactions/multisign_payment.rs lines 81–90:
https://github.com/XRPLF/xrpl-rust/blob/6031ad2/tests/transactions/multisign_payment.rs#L81-L90

// multisign() merges the signer-signed copies into the master transaction.
// Box::leak the Vec because multisign's signature ties the borrow's lifetime
// to T's 'a parameter, which the async block infers as 'static when the
// Payment was built from string literals (e.g. XRPAmount::from("20000000")).
// Without 'static here the borrow checker rejects the call. The leak is
// intentional and bounded by the test runtime.
let signer_signed_copies: &'static Vec<_> =
    Box::leak(Box::new(vec![payment_signed_by_a, payment_signed_by_b]));
xrpl::transaction::multisign(&mut payment, signer_signed_copies)
    .expect("multisign combine");

Every external caller (not just tests) will hit this. Box::leak is not an
acceptable steady state for library users.

Proposed fixes

Option A — decouple borrow lifetime, accept a slice (minimal change)

pub fn multisign<'a, 'b, T, F>(transaction: &mut T, tx_list: &'b [T]) -> XRPLHelperResult<()>
where
    F: IntoEnumIterator + Serialize + Debug + PartialEq + 'a,
    T: Transaction<'a, F>,

'b is the short borrow lifetime; 'a remains Transaction's data lifetime.
The function body never stores tx_list past the call — it iterates once
and clones the first signer of each — so a short borrow is sufficient.

Bonus: switching &Vec<T> → &[T] is more idiomatic, and existing callers
passing &vec continue to work via Vec → slice coercion.

Option B — take an iterator, consuming the txs (more flexible)

pub fn multisign<'a, T, F, I>(transaction: &mut T, tx_list: I) -> XRPLHelperResult<()>
where
    I: IntoIterator<Item = T>,
    T: Transaction<'a, F> + Clone,
    F: IntoEnumIterator + Serialize + Debug + PartialEq + 'a,

Function consumes each signer-tx; no borrow at all. Callers pass
vec![a, b] directly. Trade-off: the call site now moves the Vec rather
than borrowing it, so callers can't reuse it afterward (rarely needed
here — the signer copies exist only to be merged).

Slightly larger refactor: tx_list: I is consumed, and the loop body
already does decoded_tx_signers.push(tx_signer.clone()) on the first
signer of each tx, so adapting from for tx in tx_list (which currently
borrows each element since tx_list: &Vec<T>) to for tx in tx_list
(now owning each element via IntoIterator) requires either
tx.get_common_fields() to remain valid on an owned T (it does), or
threading borrows differently. Either way, no borrow lifetime propagates
back to the caller.

Recommendation

Option A is the smallest backward-compatible change; option B is
structurally cleaner. Both eliminate the Box::leak requirement. I'd ship
A as a zero-risk fix and consider B alongside any future refactor of the
multisign ergonomics.

Acceptance

  • Change the signature in src/transaction/multisign.rs to remove the
    borrow-lifetime / Transaction-lifetime coupling (option A or B).
  • Update tests/transactions/multisign_payment.rs (PR feat: enforce 65% integration-test coverage and add tests #298, lines
    81–90) to drop the Box::leak.
  • Verify the existing unit test in src/transaction/multisign.rs::tests
    still compiles.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions