corda / accounts

Accounts on Corda
Other
36 stars 38 forks source link

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

Open opticyclic opened 5 years ago

opticyclic commented 5 years ago

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.

See this repo for code demonstrating the issue.

roger-that-dev commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

jquijoy commented 5 years ago

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 commented 5 years ago

You need to share the account infos