RGB-WG / rgb-std

RGB standard libs for WASM & low-level integrations (no FS/networking)
https://rgb.tech
Apache License 2.0
66 stars 30 forks source link

fix bug: when build consign, override terminal #200

Closed eauxxs closed 5 months ago

eauxxs commented 6 months ago

describe: When building the consignment file, if multiple seals exist, the previous ones will be overwritten.

dr-orlovsky commented 5 months ago

Thank you for the fix and congrats with the first contribution

zoedberg commented 5 months ago

@dr-orlovsky @eauxxs Could you please explain what bug is this PR fixing? From what I see with this code we are not re-inserting a terminal seal in case there's already one associated to the same bundle ID. Before we were overriding it. But in both cases we are not warning the user that he's trying to insert a terminal seal for an already existing bundle ID. So how can we be sure the first one that we inserted was the correct one? Shouldn't we return an error if a terminal seal for a bundle ID already exists but doesn't match the terminal seal we're trying to insert?

dr-orlovsky commented 5 months ago

Basically, nothing should prevent from having multiple terminals in the same batch, since batch by definition can have multiple state transitions under the same contract, and each state transitions can have multiple outputs - and any of those outputs may be a terminal.

eauxxs commented 5 months ago

@dr-orlovsky Thank you Dr. maxim for your explanation. Yes, This is the bug I found is when I try to put multiple state transitions under the same contract into a batch. In addition, please check whether there is the same problem in this line. https://github.com/RGB-WG/rgb-std/blob/f43133f685a2a82a47c9f8ee2b67ac6df485b47a/src/persistence/stock.rs#L693-L694 If this problem does exist, can you fix it in hand, I don't think I need to repeatedly submit PR. If this problem does not exist, please ignore me.

zoedberg commented 5 months ago

Basically, nothing should prevent from having multiple terminals in the same batch

@dr-orlovsky I agree with this, but to me it seems that the code in this PR is keeping the existing Terminal in case there's already one associated to the same bundle ID:

terminals
    .entry(bundle_id)
    .or_insert(Terminal::new(seal.map(TerminalSeal::from)))

Based on what you say it seems we should change terminals from a BTreeMap::<BundleId, Terminal> to a BTreeMap::<BundleId, Vec<Terminal>>, to allow collecting more terminals in the same batch without overriding already existing ones (as it was doing before this PR) and without ignoring new additions (as it's doing with this PR).

eauxxs commented 5 months ago

@zoedberg terminal's type is hashset, It can contain multiple seals, and there is no need to use Vec\<Terminal>. https://github.com/RGB-WG/rgb-std/blob/7ad91ed137630fcfd28e5508aa3ca604396cec77/src/containers/util.rs#L54-L56

zoedberg commented 5 months ago

@eauxxs Terminal is a struct, not a hashset, seals is a hashset, but now I see the new seals are pushed on top of the ones of the existing terminal, so the current implementation should already handle all the cases correctly. Sorry for noticing this only now