Skip to content
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

accept_0conf: uncomment code to show allocations bug #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zoedberg
Copy link
Collaborator

To be merged once we have a fix for this. Run cargo test accept_0conf to see the issue:

thread 'accept_0conf' panicked at tests/utils/helpers.rs:947:17:
assertion `left == right` failed
  left: 2
 right: 1

i.e. the sender sees 2 allocations (the issued amount and the change amount) instead of just 1 (the change amount)

@dr-orlovsky
Copy link
Member

It seems the issue is not related to 0 conformations, but instead the sender sees initial issuance which has no witness transaction at all (since it is in genesis). Am I right?

Also it might be that the issuance is reported since it is a public state. Are there other tests which check that the issuance is not reported if not owned by a wallet?

@zoedberg
Copy link
Collaborator Author

It seems the issue is not related to 0 conformations, but instead the sender sees initial issuance which has no witness transaction at all (since it is in genesis). Am I right?

I don't think so. The issuance doesn't need to do a new witness transaction but it requires an existing UTXO that the wallet sees as owned and should see as unowned when it spends that. The issue is related to 0 confirmations, if you add a mine like so

    let (consignment, _) = wlt_1.transfer(invoice.clone(), None, None);

    wlt_2.accept_transfer(consignment.clone());

+   mine(false);
    wlt_1.sync();
    wlt_1.check_allocations(
        contract_id,
        &iface_type_name,
        AssetSchema::Nia,
        vec![issue_supply - amt],
        false,
    );

the test passes

Also it might be that the issuance is reported since it is a public state. Are there other tests which check that the issuance is not reported if not owned by a wallet?

As just shown, if we mine, the issuance is not reported anymore as owned. Also it wouldn't make sense, we're asking for owned allocations, not for public state.

@dr-orlovsky
Copy link
Member

i.e. the sender sees 2 allocations (the issued amount and the change amount) instead of just 1 (the change amount)

Unless the transaction is mined, in order to enable RBFs, we need to show that allocation. Thus, I think the test should be rather changed than we need to change the way allocations are presented.

The wallet receives witness txid as a part of the allocation information. With that it can use ContractState::witness_info to access the mining status and filter out mempool transactions if needed for the UX

@dr-orlovsky dr-orlovsky added the question Further information is requested label Sep 18, 2024
@zoedberg
Copy link
Collaborator Author

The tests are showing the allocations in the same way the CLI does and I think it could be misleading.
I can't find any ContractState::witness_info method, could you please show me where it is?

@dr-orlovsky
Copy link
Member

The tests are showing the allocations in the same way the CLI does and I think it could be misleading.

I think it will be good to add the information about the mining status/confirmations to the cli

I can't find any ContractState::witness_info method, could you please show me where it is?

https://github.com/RGB-WG/rgb-std/pull/269/files#diff-c9d11eb7aab562be404a98e2ffb1f9915bf3e820398303a35f1befc3c903c7dfR367

@zoedberg
Copy link
Collaborator Author

I think it will be good to add the information about the mining status/confirmations to the cli

I agree, and I think we should do the same here

https://github.com/RGB-WG/rgb-std/pull/269/files#diff-c9d11eb7aab562be404a98e2ffb1f9915bf3e820398303a35f1befc3c903c7dfR367

Ok, thanks, I thoght I should have searched in the beta 8 code. Let's leave this PR open then, when next release will be there I will use that method to add more info about mining status

@dr-orlovsky
Copy link
Member

BTW the mining status was added to the clii in RGB-WG/rgb#253

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

2 participants