corda / accounts

Accounts on Corda
Other
36 stars 38 forks source link

Incorrect result while transferring token from Node containing account #87

Closed saraGeorge closed 4 years ago

saraGeorge commented 4 years ago

Hi,

I have deployed a cordapp with 2 nodes A and B and a Notary.

I have created an account on node A .

Hence following is my configuration Node A -> Account A1

Process : Step 1: Issue 100.00 USD to Node A to Node B : Expected Balances: Node A - 0, Account A1 -0 , Node B -100 Actual Balances : Node A - 0, Account A1 -0 , Node B -100

Step 2: Issue 70.00 USD to Node A to Account A1 : Expected Balances: Node A - 0, Account A1 -70 , Node B -100 Actual Balances : Node A - 0, Account A1 -70 , Node B -100

Step 3: Transfer 5 USD from Node B to Node A. Expected Balances: Node A - 5, Account A1 -70 , Node B -95 Actual Balances : Node A - 5, Account A1 -70 , Node B -95

Step 4: Transfer 2 USD from Node A to Account A1 : Expected Balances: Node A - 3, Account A1 -70, 2 , Node B -95 Actual Balances : Node A - 3, Account A1 -70, 2 , Node B -95

Step 5: Transfer 9 USD from Node A to Account A1 : Expected Balances: Error as Node A doesn't have expected tokens Actual Balances : Node A - 5, 68, Account A1 - 2 , Node B -95

I want to transfer tokens from the node A and not the account A.

I have tried writing criteria expression as follows :

Field checkForHostField = PersistentFungibleToken.class.getDeclaredField("holder"); CriteriaExpression hostIndex = Builder.equal( checkForHostField, getOurIdentity(), true); QueryCriteria hostCriteria = new QueryCriteria.VaultCustomQueryCriteria(hostIndex); QueryCriteria criteria = unconsumedStatesCriteria.and(hostCriteria);

This query criteria however fetches fungible token states where both account and Node is the token holder after step 3.

When I tried to query heldTokenAmountCriteria(token, nodeAIdentity)) I got the same result as mentioned above, but I want to retrieve tokens held by NodeA and not an account in NodeA.

Can somebody please explain what is going on in the above scenario? Is there anyway to mitigate this error and ensure that the tokens are indeed deducted from the expected accounts.

Thanks in advance

adelrustum commented 4 years ago
  1. Here's the right way to query by accounts: https://github.com/corda/accounts/blob/master/docs.md#querying-the-vault-by-account
  2. As per the official documentation, it is not advisable to mix accounts with non-accounts. It is recommended to create a "default" account for your node instead: https://github.com/corda/accounts/blob/master/docs.md#how-do-we-mix-workflows-with-accounts-and-non-accounts
    Currently, if accounts and non-accounts workflows are mixed on the same node then 
    you need to be careful with state selection. When selecting states for non-accounts
    workflows, the state selection code will currently pick states which could be
    assigned to accounts.
roger-that-dev commented 4 years ago

Thanks @adelRestom - that's right. It's because by default, the vault query API doesn't know anything about accounts. It assumes that all tokens/states are "up for grabs" when it comes to queries and therefore selection. What you need to do is be explicit regarding where to select the tokens from. Now, with an account that's easy - you can use, this as part of your query criteria:

UUID uuid = UUID.fromString(accountId);
QueryCriteria.VaultQueryCriteria vaultQueryCriteria = new QueryCriteria.VaultQueryCriteria().withExternalIds(Collections.singletonList(uuid));

Where uuid is the account ID. However, if you just want to select tokens from the node (or any of its confidential identities) then that's actually possible at the moment (see above).

A workaround might be to set a default account for the node when the node starts up for the first time and issue tokens to that account instead of the node's legal identity key or a confidential identity belonging to the node operator. Whilst it takes a bit more effort to set up the account, this approach would solve your problem.

In future versions I suspect we can fix this.

Thanks for raising the issue.

adelrustum commented 4 years ago

Sara, also another very important thing; always specify the change holder when you call Tokens flows; otherwise the change goes by default to the flow initiator. So if you wanted to transfer quantity 2 from Account-A and it only had one toke of quantity 5, that token will be consumed, if you don't specify Account-A (in the MoveFungibleToken flow) then the change will go to the flow initiator (i.e. NodeA); so the balance of Account-A will become 0.

adelrustum commented 4 years ago

Roger, can you (as an improvement) make the change holder a mandatory field in the constructor? This will remind the developer to supply that value.

saraGeorge commented 4 years ago

Hi, I have already made changeHolder a mandatory field.

saraGeorge commented 4 years ago

Thankyou for your replies

roger-that-dev commented 4 years ago

Unfortunately we can't do this without breaking backwards compatibility but if no-one cares about compatibility at this stage, we can do it. Thoughts?

adelrustum commented 4 years ago

I don't think backward compatibility is an issue here, in a sense that we're not changing how a certain flow is working; we're just requesting that a certain attribute value must be supplied (i.e. changeHolder).
So worst case scenario, the developer will get a compilation error when upgrading to the new SDK version.

roger-that-dev commented 4 years ago

Yeah, that's a compatibility break :) Some people are quite passionate about compatibility and we also said we would aim to maintain it as much as possible. None the less, I think you are right and it does make sense to make the parameter required.

adelrustum commented 4 years ago

How about you introduce a new constructor and mark the old one as deprecated?