`TxGroup::assign_group_id` should take a `&mut [Transaction]` or `&mut Vec<_>` instead of a `Vec<&mut Transaction>`.

[AlterionX]

Is your feature request related to a problem? Please describe.
As a general rule, so far as I can tell, interfaces should use either an iterator or a slice unless you're taking ownership of the data held within. In this case, since assign_group_id only modifies the data provided, I believe this is a better approach.

Describe the solution you'd like
Replace the parameter type of assign_group_id's (Vec<&mut Transaction>) with &mut [Transaction]

Describe alternatives you've considered
It's possible that this is still kind of restrictive. A more obtuse, but more flexible approach is to take in an impl IntoIterator<Item = &mut Transaction>. Both Vec<&mut Transaction> and &mut [Transaction] (and &[&mut Transaction]) should implement this or have conversion methods readily available.

Again, this is a bit obtuse, though.

Additional context
To recap, I'd like:

pub fn assign_group_id(txns: &mut [Transaction]) -> Result<(), TransactionError> {
    let gid = TxGroup::compute_group_id(txns)?;
    for tx in txns {
        tx.assign_group_id(gid);
    }
    Ok(())
}

(I haven't checked if tweaking compute_group_id would affect any other functions.)
OR

pub fn assign_group_id<'a>(txns: impl IntoIterator<Item = &'a mut [Transaction]>) -> Result<(), TransactionError> {
    let txns: Vec<_> = txns.into_iter().collect();
    let gid = TxGroup::compute_group_id(txns.as_slice())?;
    for tx in txns {
        tx.assign_group_id(gid);
    }
    Ok(())
}

OR

pub fn assign_group_id<'a>(txns: impl IntoIterator<IntoIter = impl Iterator<Item = &'a mut Transaction> + Clone>) -> Result<(), TransactionError> {
    let txn_iter = txns.into_iter();
    let gid = TxGroup::compute_group_id(txn_iter.clone())?;
    for tx in txn_iter {
        tx.assign_group_id(gid);
    }
    Ok(())
}

(I... don't think I can recommend this last one for a general purpose interface. The Clone potentially hides a lot of work. Maybe a Copy might be better? You can't just pass a reference since the iterator will get consumed.)

EDIT: Changed the IntoIterator<Item = &'a mut [Transaction]> into IntoIterator<Item = &'a mut Transaction>.

[manuelmauro]

We certainly should abandon the 'Vec<&mut Transaction>` parameter. I gave a try to your slice suggestion and I find it a good solution. The Iterator solution maybe would make the API less readable? Do you have any example of this approach in the standard library or popular libraries?

[ivnsch]

The reason that option 1) is not being used IIRC is that you transfer the transactions ownership to the vector, so to continue using them after assigning the id, you've to get them back from it:

let txs = &mut vec![t1, t2];
TxGroup::assign_group_id(txs)?;
let signed_t1 = account.sign_transaction(&txs[0])?;
let signed_t2 = account.sign_transaction(&txs[1])?;

which is slightly error prone and not so nice in general.
Also, having to declare the transactions themselves as mutable, while a bit tedious, signalizes intent better IMO.
The owned vector parameter, while not idiomatic can be seen as a throw away vehicle to mutate the transactions.
Might be missing something.

The second variant allows to keep the caller side similar to what we've now, but it seems to do more work than assign_group_id(txns: Vec<&mut Transaction>), which is likely negligible due to the number of transactions normally passed / the max group size, but makes me question it as a replacement (combined with the more complicated signature).

The purpose of the third variant isn't quite clear.

[manuelmauro]

Would something like this be a reasonable compromise?

pub fn assign_group_id(txns: &mut [&mut Transaction]) -> Result<(), TransactionError> {
        let gid = TxGroup::compute_group_id(txns)?;
        for tx in txns {
            tx.assign_group_id(gid);
        }
        Ok(())
    }

only difference in the atomic_swap example would be a mutable slice instead of a vec! macro

    TxGroup::assign_group_id(&mut [t1, t2])?;

    let signed_t1 = account1.sign_transaction(&t1)?;
    let signed_t2 = account2.sign_transaction(&t2)?;

which does not move t1 and t2.

Notice that something like

    let mut txs = vec![t1, t2];
    TxGroup::assign_group_id(txs.as_mut_slice())?;

    let signed_t1 = account1.sign_transaction(&txs[0])?;
    let signed_t2 = account2.sign_transaction(&txs[1])?;

would work and the user is explicitly deciding to move the transactions within a Vec.

[AlterionX]

The Iterator solution maybe would make the API less readable? Do you have any example of this approach in the standard library or popular libraries?

It's mostly found in libraries built for iterator manipulation. One example is itertools::join. I've also seen it in nalgebra::from_iterator. It's definitely not common, but it's a pattern I use extensively throughout my code. Perhaps we could create variants for ease of use? (Copying how the std would create specialized functions for certain common types.)

fn assign_group_id<'a>(txns: Vec<&'a mut Transaction>) {
    assign_group_id_iter(txns)
}

fn assign_group_id_iter<'a>(iterator: impl IntoIterator<Item = &'a mut Transaction> + Clone) {
    // ...
}

It's rather similar to an API taking impl AsRef<str>.

---

The purpose of the third variant isn't quite clear.

Uh, just realized, I meant IntoIterator<IntoIter = impl Iterator<Transaction<Item = &mut Transaction> + Clone>> not IntoIterator<Item = &mut [Transaction]> + Clone. Does that change things?

The third variant exists to avoid that additional work mentioned about the second variant. As a general rule, Iterators over references are cheap to clone. To make it more restricted, the parameter would IntoIterator<Item = &mut Transaction> + Copy

I know that variant 2 & 3 are obtuse, which is why I'd suggested the variant 1 instead of 2 & 3.

Another note is that the signature would accept all of the following:

TxGroup::assign_group_id([&mut t1, &mut t2]);
let txn: Vec<Transaction> = vec![];
TxGroup::assign_group_id(txns); // Probably wrong. Also disallowed by `Copy` variant
TxGroup::assign_group_id(&mut txns);
TxGroup::assign_group_id(txns.iter_mut()); // Disallowed by `Copy` variant
TxGroup::assign_group_id(txns.as_mut_slice());
let txn: Vec<&mut Transaction> = vec![];
TxGroup::assign_group_id(txns); // Disallowed by `Copy` variant
TxGroup::assign_group_id(&txns);
TxGroup::assign_group_id(txns.iter()); // Disallowed by `Copy` variant
TxGroup::assign_group_id(txns.as_mut_slice());

---

Would something like this be a reasonable compromise?

I like it!

---

In my case, I'm orchestrating transactions of many NFT/limited count tokens between multiple parties, so my use case ends up looking a little like this:

fn attempt_transfer(transfer: Transfer) -> {
    let mut txns = transfer.construct_transactions();
    // What I do now
    TxGroup::assign_group_id(txns.iter_mut().collect());
    // This would be nice
    TxGroup::assign_group_id(&mut txns);
    // This was mentioned, and I like it.
    TxGroup::assign_group_id(txns.as_mut_slice());

    // Does a bit of look up & stuff to figure out who is supposed to sign what.
    // Also takes ownership of the transaction, and pumps out the signed versions for broadcast.
    let signed_txns = transfer.sign_transactions(txns);
}

And the other use case I was imagining

    // But maybe it's a HashMap<_, Vec<Transaction>> instead, so it'd be nice to
    let txn_refs = txns.values_mut().flat_map(|txns| txns.iter_mut());
    TxGroup::assign_group_id(txn_refs);
    // But as is, I need to collect.
    let txn_refs = txns.values_mut().flat_map(|txns| txns.iter_mut()).collect();
    TxGroup::assign_group_id(txn_refs);

In the long run, the compiler probably knows about how to handle it, but I'd like to avoid potentially costly collects.

I'm simulating guilds of players pooling on chain resources to do things at the moment, so there are a lot of transactions going on. Maybe I should look at a different way to do things?

[ivnsch]

+1 for &mut [&mut Transaction]. It seems the "correct version" of what we've now and provide enough flexibility.

I'm simulating guilds of players pooling on chain resources to do things at the moment, so there are a lot of transactions going on. Maybe I should look at a different way to do things?

This goes a bit beyond the scope of this issue / possibly the SDK. I'd recommend asking first in the Algorand forum or Discord.

[AlterionX]

Okay. I'll make that change, then, since it looks like we're all agreed.

---

This goes a bit beyond the scope of this issue...

Ah, yeah, I wasn't really asking for help with it, more that I'm trying to establish why I'd like something more generic.

[dallin-shogun]

Looking forward to the PR on this, just got stuck on this exact issue.

[ivnsch]

@AlterionX @manuelmauro feel free to double check that it's what we agreed.

@dallin-shogun you can reference the PR branch for now.

[AlterionX]

Looks good to me.

[ivnsch]

Thanks everyone for the collaboration!

[manuelmauro]

Thanks everyone!