Skip to content
This repository was archived by the owner on Dec 9, 2023. It is now read-only.

Proposal to unlocking blind utxo #204

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

crisdut
Copy link
Member

@crisdut crisdut commented Sep 14, 2022

In addition to the changes in #195, I decided to make a change in consume_transfer to unlocking utxo blinding.

@crisdut crisdut force-pushed the feat/consume-reveal branch from fcedeff to f666b79 Compare September 15, 2022 00:29
@crisdut
Copy link
Member Author

crisdut commented Sep 15, 2022

Hi @zoedberg , I found the bug.

I did not save the new transition in transitions and transition_txid tables. Also, I added the CloseMethod property at the Reveal object.

Contract state before spend asset:

image

Contract state after spend asset:

image

I was able to spend an asset received, and I used the change address twice.

I made the tests with branch bellow (I merged the fix/process_consignment branch with feat/consume-reveal)

PS: I tested only tapret dbc. I will start tests with opret (I never used this).

@crisdut
Copy link
Member Author

crisdut commented Sep 15, 2022

@dr-orlovsky this branch only can merged after merge branch #195.

Thanks!

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits, also, can you create steps in the demo-rgb.sh to test this as a form of smoke test?

@zoedberg
Copy link
Contributor

thanks @crisdut, I confirm you solved the issue I reported and now all rgb-lib tests are passing :)

@crisdut crisdut force-pushed the feat/consume-reveal branch from f666b79 to 710260c Compare September 15, 2022 14:34
@crisdut
Copy link
Member Author

crisdut commented Sep 15, 2022

Just some nits, also, can you create steps in the demo-rgb.sh to test this as a form of smoke test?

Thanks for reviewing @cryptoquick!

I will start writing the steps, and I'll let you know when I finish them.

@crisdut
Copy link
Member Author

crisdut commented Sep 15, 2022

Hi @cryptoquick,

The instructions: https://github.com/crisdut/nodes/tree/feat/unlocking-blind-utxo

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reviewed your changes. Looks good!

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized something, the lints are failing.
Be sure to run: cargo clippy

@crisdut crisdut force-pushed the feat/consume-reveal branch from 710260c to 8ba7077 Compare September 15, 2022 21:39
@dr-orlovsky
Copy link
Member

dr-orlovsky commented Sep 18, 2022

I have merged #195, so I assume this needs rebase.

Regarding the question from there, which has caused this PR: why would we need two methods instead of one? Well, there was a clear reason, since I have started with just one method and it didn't worked that way, so I had to made two. I am currently sick and my head is not working properly, so I can't recall what was the problem that caused splitting method in two. It could be that it was related to multisig wallets and interactivity required for managing stash under multisig wallets, such that all parties should do one of these two procedures before any of them would proceed to the second one - or something like that. I will try to re-analyse and recall better once I will recover.

@dr-orlovsky dr-orlovsky added the question Further information is requested label Sep 18, 2022
@crisdut
Copy link
Member Author

crisdut commented Sep 18, 2022

I have merged #195, so I assume this needs rebase.

Yes, you are correct.

Regarding the question from there, which has caused this PR: why would we need two methods instead of one?

I am unsure about this question because I followed the recommendations about extending the method rgb-cli consume instead of creating a new method to reveal the confidential seal.

If your question refers to why rgb-cli finalize --send is not enough to transfer and reveal the confidential seal, I think there is one reason. To spend the receive assets with rgb-cli transfer compose, the receiver needs unlocking the confidential seal with blinding factor and receive txid:vout.

@dr-orlovsky
Copy link
Member

You got me wrong: it was not my question but a quote of Zoe question from here #195 (comment)

And I tried to answer that

@crisdut crisdut force-pushed the feat/consume-reveal branch from 8ba7077 to d3b08b7 Compare September 24, 2022 23:04
@crisdut
Copy link
Member Author

crisdut commented Sep 25, 2022

Hi @dr-orlovsky,

I fix the PR.

Can you review this code, please?

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial undetstanding was that RGB Node has to keep the blind UTXO secrets it had generated before and use them internally not requiring the client to provide. I do not remember at this moment does the blind UTXO created by the node - or using RGB libs on a wallet side. If the first is correct, than I would have to re-work this PR and complete that workflow. If not, this PR is overall correct - excluding few points I address in the code review comments

@crisdut
Copy link
Member Author

crisdut commented Sep 28, 2022

My initial undetstanding was that RGB Node has to keep the blind UTXO secrets it had generated before and use them internally not requiring the client to provide. I do not remember at this moment does the blind UTXO created by the node - or using RGB libs on a wallet side.

Currently, the blind utxo is created with rgb-std lib. I think this process is moved to rgb-std after version v0.8.0 of the rgb-node.

If not, this PR is overall correct - excluding few points I address in the code review comments

Done

Copy link
Member

@cryptoquick cryptoquick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like all the linter checks have passed. Clippy is super helpful. LGTM!

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 2954e69

@dr-orlovsky dr-orlovsky merged commit 56cf7c4 into RGB-WG:master Sep 30, 2022
@dr-orlovsky
Copy link
Member

Merged. Thank you very much for fixing this thing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants