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

Can't Update States As Can't Find Accounts #65

Open
opticyclic opened this issue Sep 30, 2019 · 8 comments
Open

Can't Update States As Can't Find Accounts #65

opticyclic opened this issue Sep 30, 2019 · 8 comments

Comments

@opticyclic
Copy link
Contributor

I can create a state using accounts but I can't subsequently update that state in a different flow.
The account cannot be found in the second flow even though it was found in the first flow.

  • Create 2 nodes
  • Create an account on each node
  • Share accounts across nodes
  • Create an IOU between accounts
  • Verify that you can find both accounts on both nodes by UUID
  • Try to update the IOU
  • Fails because you can't find the account

See this repo for code demonstrating the issue.

@roger-that-dev
Copy link

roger-that-dev commented Oct 10, 2019

This isn't a bug. I've checked out out your code and run it. The test is failing because you are not sharing the lender (bank) key with the borrow (agent). Or rather.. the agent is not storing a mapping of the new key generated by the bank node to the bank node's x500name. It was useful to look at your code and see where we can make accounts easier to use. I'll make some changes to the code and maybe it'll be easier for you in the future so you won't run into these mistakes. Cheers

@roger-that-dev
Copy link

It's also worth noting that the lender AccountInfo can be found in the first "flow" because the initiator part of the flow where all the logic is being executed, is being run by the lender node. The update flow fails because it is being run by the bank node, which doesn't have the mapping.

@opticyclic
Copy link
Contributor Author

Can you clarify in the docs what the ShareAccountInfoWithParty does and doesn't do.

    /**
     * Shares an [AccountInfo] [StateAndRef] with the specified [Party]. The [AccountInfo]is always stored by the
     * recipient using [StatesToRecord.ALL_VISIBLE].
     *
     * @param accountId the account ID of the [AccountInfo] to share.
     * @param party the [Party] to share the [AccountInfo] with.
     */
    fun shareAccountInfoWithParty(accountId: UUID, party: Party): CordaFuture<Unit>

I expected that this would be enough to share the lender with the agent.

I subsequently found this comment in another demo from a few months ago:

    //Share the state and the account with the Bank node so that we can look up by keys
    //TODO: The test will fail if we don't do this. Ideally the framework should do this for us
    banksAccountService.shareStateAndSyncAccounts(signedTx.tx.outRefsOfType<IOUAccountState>().single(), agents.identity()).runAndGet(network)

Quite clearly, it is easy to forget to do this extra flow, so now I am doing this at the end of the issue flow:

        //Notarise and record the transaction in both parties' vaults.
        progressTracker.currentStep = FINALISING
        val transaction = subFlow(FinalityFlow(fullySignedTx, listOf(borrowerSession)))
        val state = transaction.tx.outRefsOfType<IOUAccountState>().single()
        subFlow(ShareStateAndSyncAccounts(state, borrowerAccountInfo.state.data.host))
        return transaction

Is this the right way to do it?
Could this sharing be done automatically?

@roger-that-dev
Copy link

You are right - the usability could be improved. However, this is just a V1 so I guess we won't get it 100% right the first time. I'm going to see what I can do to improve this before the release. Thanks

@roger-that-dev
Copy link

Thinking about this a little, maybe what we need is a version of the SyncKeyMappingFlow that also adds PublicKey- > Account ID mappings. Maybe could be called sync account keys flow and can be used when a node already has the AccountInfo but they are missing some of the key mappings. The mappings can be extracted from a state/transaction or just provided as a list of pairs, or something. Would that help ?

@opticyclic
Copy link
Contributor Author

That sounds like it should work.
Obviously, its hard to tell without implementing it and trying it out!

@jquijoy
Copy link

jquijoy commented Oct 31, 2019

Hi all, I am also facing this kind of issue with the project I am currently working on. Is there any available solution for this?

@roger-that-dev
Copy link

You need to share the account infos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants