Allow multiple transfers by CLI

[crisdut]

I updated the current rgb-cli transfer finalize to support multiple transfers with bifrost send option.

Before:

rgb-cli transfer finalize {PSBT} {CONSIGMENT} --endseal {SEAL_DEFINITION} --send {BIFROST_ADDR}

Now:

rgb-cli transfer finalize {PSBT}  --endseal "{SEAL_A}[,{SEAL_B}]:{CONSIGMENT_A}" --endseal "{SEAL_C}:{CONSIGMENT_B}" ...

Example:

rgb-cli transfer finalize /var/lib/rgb/atomic.psbt --endseal "$bob_seal,$charlie_seal:/var/lib/rgb/bob.rgbc" --endseal "$alice_seal,:/var/lib/rgb/alice.rgbc" ...

PS: @zoedberg I didn't modify finalize_transfers because I was afraid of breaking something in rgb-lib.

[cryptoquick]

I wouldn't worry about making breaking changes in strongly-typed APIs, so long as semver rules are respected. Rust will let developers know they need to make updates. I'd prioritize making the optimal API and maintainable, de-duplicated code instead of worrying about downstream users, at this stage (pre-1.0), at least.

[crisdut]

I wouldn't worry about making breaking changes in strongly-typed APIs, so long as semver rules are respected. Rust will let developers know they need to make updates. I'd prioritize making the optimal API and maintainable, de-duplicated code instead of worrying about downstream users, at this stage (pre-1.0), at least.

This is a good advice, thanks!

[zoedberg]

Hi @crisdut!

This might not be the best approach. Given the RGB invoice could contain multiple endpoints, each with its own protocol, I don't think rgb-node APIs should use storm directly. Actually I think we should completely remove connections between rgb-node and storm, in order to allow more generalized exchange mediums for consignments.

In order to achieve the same result of this PR, I propose to edit the finalize CLI command in order to accept a list of end seals only having seal and consignment (without the bifrost endpoint) and to add a separate CLI command (on storm) to send consignments to storm endpoints.

[crisdut]

Hi @zoedberg

Hi @crisdut!

This might not be the best approach. Given the RGB invoice could contain multiple endpoints, each with its own protocol, I don't think rgb-node APIs should use storm directly. Actually I think we should completely remove connections between rgb-node and storm, in order to allow more generalized exchange mediums for consignments.

I saw your PR to allow the flexibility of transport protocols to consignment files. For now, I think removing the integration between RGB and STORM makes sense, but I would like to discuss a way to propagate consignment files to guarantee a global state (I was working on new integrations between rgb and storm). I will open a discussion later.

In order to achieve the same result of this PR, I propose to edit the finalize CLI command in order to accept a list of end seals only having seal and consignment (without the bifrost endpoint) and to add a separate CLI command (on storm) to send consignments to storm endpoints.

I will make the change ASAP

Thanks for feedback!

[crisdut]

Hi @zoedberg, I made the changes.

Can you review it again, please?

I will open a new PR to remove the code related to Storm, but I would like to know if @dr-orlovsky has plans to maintain some integration between the nodes.

[zoedberg]

Can you review it again, please?

I've reviewed it and left some comments. I don't have the time right now to test it but I assume you already verified it works as expected.

but I would like to know if @dr-orlovsky has plans to maintain some integration between the nodes.

I'm convinced that separating rgb-node from storm is the best architectural choice, so I'd like to first have confirmation from @dr-orlovsky that we're on the same page in this regard. Users of rgb-node should not be forced to also run storm, since it's not required to operate with RGB. In my opinion, any integration between rgb-node and storm should be optional (e.g. via cargo features) or implemented in separate projects.

[zoedberg]

@crisdut there are some commented lines in cli/src/opts.rs that can be removed but other than that the PR looks good now so I'll approve it, nice job

[dr-orlovsky]

1. I do like the idea of separating storm form RGB Node

2. This should not create any problems with access to a global state since the state necessary for the validation must always be provided by a creator of the consignment as a part of consignment. If not, the consignment is simply invalid.

3. It is not clear form me why we need to process multiple consignments at once and why we cant't just do a while loop with shell script. Putting complex structured data into a single parameter is much less preferable for that, so concept NACK regarding this PR, sorry. But I am opened to counter-arguments if there are use cases for this which can't be handled with shell scripting.

[cryptoquick]

3. It is not clear form me why we need to process multiple consignments at once and why we cant't just do a while loop with shell script. Putting complex structured data into a single parameter is much less preferable for that, so concept NACK regarding this PR, sorry. But I am opened to counter-arguments if there are use cases for this which can't be handled with shell scripting.

This is a good point... Since the consignment is persisted on-disk and this retains state between invocations, multiple transfers can be added to it just by running the command multiple times using different parameters. Is this what you mean, @dr-orlovsky ?

[zoedberg]

1. I do like the idea of separating storm form RGB Node

2. This should not create any problems with access to a global state since the state necessary for the validation must always be provided by a creator of the consignment as a part of consignment. If not, the consignment is simply invalid.

3. It is not clear form me why we need to process multiple consignments at once and why we cant't just do a while loop with shell script. Putting complex structured data into a single parameter is much less preferable for that, so concept NACK regarding this PR, sorry. But I am opened to counter-arguments if there are use cases for this which can't be handled with shell scripting.

1. Happy to be on the same page :)
2. I agree
3. Using the finalize_transfer API and handling this via bash script is possible but not so easy/clean. This API calls Anchor::commit that cannot be called twice on the same psbt (it produces an error saying output already contains commitment; there must be a single commitment per output.), so in order to avoid failures the bash script should not override the original psbt (at least not until the end of the for loop). Moreover in that method there are some operations that can be called only once per "batch transfer" (i.e. single tx sending multiple assets), for example bundles extraction and disclosure saving. For these reasons, plus the fact that currently we have 2 APIs that are very similar (finalize_transfer and finalize_transfers) with no reason to keep both, I think we should drop finalize_transfer and accept this PR

[dr-orlovsky]

Ok I see now (re pt 3). I revoke my "Concept NACK". Though I still think the way it is packed into a single command parameter is hard to deal with. Will think on a better approach.

So: Concept ACK, approach NACK modulo if we do not find a better one.

[crisdut]

Ok I see now (re pt 3). I revoke my "Concept NACK". Though I still think the way it is packed into a single command parameter is hard to deal with. Will think on a better approach.

So: Concept ACK, approach NACK modulo if we do not find a better one.

3. My opinion is similar to @zoedberg, I tried make CLI compatible with finalize_transfers to avoid duplication of code and allowed node operators make transfer multiple assets.

[dr-orlovsky]

This API calls Anchor::commit that cannot be called twice on the same psbt (it produces an error saying output already contains commitment; there must be a single commitment per output.), so in order to avoid failures the bash script should not override the original psbt (at least not until the end of the for loop).

It seems to me that the right thing to do is to separate methods for:

1. Preparing consignment with end seals and adding all neccessary info to PSBT - called once for each consignment
2. Finalizing PSBT Tapret commitment (which can include not only RGB data but other protocols as well), called just once - and doing this Anchor::commit).

Another thing that this PR is API breaking, so it has to be done with bumping RGB Node to v0.10 and can't go because of that into v0.8.x nor v0.9.x. The question now is how urgently this PR is needed (once we agree on how it should be handled in the API).

[crisdut]

Another thing that this PR is API breaking, so it has to be done with bumping RGB Node to v0.10 and can't go because of that into v0.8.x nor v0.9.x. The question now is how urgently this PR is needed (once we agree on how it should be handled in the API).

Sure.

We can remove only finalize_transfer for now and maintain the same behavior of the cli (only send one transfer). Does make sense to you?