Change tokens supply #1179
Change tokens supply #1179
Conversation
azarovh
force-pushed
the
feat/increase_tokens_supply
branch
from
10fba62
to
b23c094
Compare
azarovh
force-pushed
the
feat/increase_tokens_supply
branch
from
59f5707
to
94822ac
Compare
azarovh
force-pushed
the
feat/increase_tokens_supply
branch
2 times, most recently
from
4b2a5fb
to
88996c1
Compare
azarovh
requested review from
muursh,
erubboli and
iljakuklic
as code owners
chainstate/tx-verifier/src/transaction_verifier/token_issuance_cache.rs
Outdated
Show resolved
Hide resolved
chainstate/tx-verifier/src/transaction_verifier/tests/hierarchy_read.rs
Outdated
Show resolved
Hide resolved
|
Besides the comments I made, this is fine. But there's a huge lack of testing. We need to test the hell out of this. The tests should include all kinds of random combinations of:
And all other possible operations. And every step in these tests should be followed by checking the tokens info/state. Before merging this, we should also add the wallet functionality. |
azarovh
force-pushed
the
feat/increase_tokens_supply
branch
from
88996c1
to
86f5df1
Compare
azarovh
force-pushed
the
feat/increase_tokens_supply
branch
from
9768935
to
6cd5f00
Compare
| .decrement() | ||
| .map_or(CachedOperation::Erase, CachedOperation::Write); | ||
| self.account_nonce.insert(account, new_nonce); | ||
| self.unspend_input_from_account(account_input)?; |
There was a problem hiding this comment.
From the code above it looks like multiple token supply change inputs are allowed, so I guess one can do mint(nonce 1) + unmint(nonce 2) in the same transaction.
In unspend_input_from_account will the nonce will not be rolled back properly, as it uses decrement from the total count ?
There was a problem hiding this comment.
From the code above it looks like multiple token supply change inputs are allowed
Yes that's allowed, I've added tests to recheck that and they work fine.
as it uses decrement from the total count
It increments and decrements per input. I think the name of the method that is used (get_account_nonce_count) is confusing. It should be just get_account_nonce
| undos | ||
| .into_inner() | ||
| .into_iter() | ||
| .try_for_each(|undo| self.tokens_accounting_cache.undo(undo))?; |
There was a problem hiding this comment.
similar concern for a mint + unmint in the same transaction and the order of the undos, are the undos processed in reverse order?
If they are not, given a scenario that we first mint 10 tokens, and then mint 20 and burn 25.
the final state should be 5 tokens, but on undo of the last transaction
if it iterates from the first input it will need to undo the mint of 20 which should fail?
There was a problem hiding this comment.
Nice catch! It actually works because the undos are associative deltas. I'll change the order for the sake of readability and make sure you case is convered in tests
There was a problem hiding this comment.
it might mess up with DeltaData, I'll check
There was a problem hiding this comment.
An interesting case that worked anyway because of the lack of checks on undo is mint + lock in the same tx, which should've failed with an error but worked anyway. I added additional checks and reversed the order of processing the undo.
Also I reversed it for pos accounting outputs, but I can't see the case to reproduce
I have to apologise for the size of this PR. It should've been split in 2 but I didn't see a good way to do it without painful rebase of multiple commits and right now it's too late.
Closes #1206
OutputValue::TokenV1type. To simplify transfer/burning of tokens and also make issuance unambiguous and authorisable.TxOutput::Tokenstype that replacesOutputValue::Transfer(TokenIssuance)with explicit purposes.tokens-accountingcrate to handle token supply logicThis PR also attempts to support the logic in the
wallet. But it was too much for me so I stubbed it in many places and also commented out some checks in tests. All places that I touched there are marked with "TODO: add support for tokens v1" as well as the link to the issue #1237