-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support CRDT to RgbWallet #353
Conversation
f12759b
to
0c3fc74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think the two errors I passed to you are related with previous errors but better be more or less sure that is that before merge. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic work and is a great start. Just got a couple minor changes requested.
Ok, sure. I think CDRT works at the "plain text" level, so the merge should happen in a step before serialization. My only question is whether the reconcile process would work with these confined vectors. Today the main bug in transfers occurs in the RgbWallet wallet, as some taprets are being saved and as this part does not use strict-types, we end up using postcard encoding. I thought of creating a version of RgbWallet supporting strict-types, to test whether adding each new element would always make the file bigger and because of that, it would be saved in carbonado. But I guess that's not the real problem. What do you think? Does this experiment make sense?
I'm not so sure, I had done some simulations of how the BME behaves (concurrent accesses on a single wallet) and inevitably we can have serious cases where two versions of the same wallet writing concurrently can end up erasing previous information. One way to eliminate this was to use unit of work and now, automerge. But the way BME calls each operation individually and concurrently can end up impacting this. Do you agree? |
|
Ok, I will make RgbWallet with strict-type in another branch to test, ok?
I implemented it in a similar way to this one, I'll detail it below:
This is the way I found that makes the most sense. Do you agree? |
|
It makes sense what you did @crisdut. It's only with one token that I created, but I created a lot more without any problems at all. And no problems after any transactions. For me is ACK for merge (and for @cryptoquick to deploy a new npm rc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Will merge this and build a new release. Any additional changes can come in on a new PR.
Description
This PR allow us use CRDT (automerge) strategy in
rgb_wallets
to avoid loose contract state in concurrency operations.Now, transfers operations pull "a fork" of the
rgb_wallet
from carbonado server to use in client-side (wasm32) and push a merged version of thergb-wallet
to carbonado server.How it works
We have two new methods: retrieve_fork_wallets and store_fork_wallets. The first method make a copy of the wallet and the other method store the merged versoin of the wallet.
Limitations
By now, we use automerge and autosurgeon crates to handle the CRDT.
Misc
In the medium term, I intend to add this functionality directly to Stock, in rgb-wallet project, if I can combine with base58 (armored).
Closes #268