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

6184 wallet.current record #6359

Merged
merged 5 commits into from
Sep 30, 2022
Merged

6184 wallet.current record #6359

merged 5 commits into from
Sep 30, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Sep 29, 2022

refs: #6184 (partial)

Description

Add a .current subnode to the wallet node for clients to have a stable HEAD that has all precious state. This is a slow growing set so won't bring on the problems of #6038

The keys are,

  • brands is a list of brand descriptors. Changes only when new purse is added. Needed for issuers, petnames, etc
  • purses is a list of purse states. There's currently one per brand, so we could do a map, but we know that in the future we'll have more than one purse per brand so let's not have to change the chain storage to accommodate that. Here we have an array of purses and additional fields can be added to distinguish.
  • offerToUsedInvitation is a map of offerId to an invitation that has been consumed. This is useful for clients to look up the offer ID for a given invitation spec, by which they can use its invitation makers.
  • lastOfferId is a way for clients to ensure they're using a sufficiently high offer ID in their next offer. This could be read from the wallet node, but if there have been many balance updates since the last offer there may be no offerStatus updates in available history.

Also updates wallet CLI and GUI to use it.

Security Considerations

Nothing exposed that wasn't published before in some fashion.

Documentation Considerations

--

Testing Considerations

PSM integration test covers it. It would be good to have more specific tests but not economical right now.

Manually tested agoric wallet show and that Wallet UI shows the purse balances.

@turadg turadg requested review from dckc and michaelfig September 29, 2022 19:18
@turadg turadg force-pushed the 6184-wallet-current branch 2 times, most recently from eb33d67 to 4d97f9c Compare September 29, 2022 23:34
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks on target.

I'm less confident about the wallet ui, so perhaps stand by for someone else to review that part.

);
t.deepEqual(Object.keys(currentState.offerToUsedInvitation), ['44']);
t.is(
currentState.offerToUsedInvitation[44].value[0].description,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bingo

@@ -47,7 +47,7 @@ agd query bank balances "$WALLET_BECH32" | grep ubld || exit 1
echo "Provisioning your smart wallet..."
agoric wallet provision --spend --account "$WALLET"
echo "waiting for blocks"
sleep 5
sleep 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I tripped on that too.

const latest = await iterator.next();
/** @type {import('@agoric/smart-wallet/src/smartWallet').CurrentWalletRecord} */
const current = latest.value.value;
console.log('fetcHCurrent', current);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks drafty. Did you mean to keep that?
Not critical.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, thanks, i'll fixup

console.log('fetchCurrent()');
const latestIterable = await E(currentFollower).getLatestIterable();
const iterator = latestIterable[Symbol.asyncIterator]();
const latest = await iterator.next();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent a race between the two followers, we need to store the height at which latest was published, like the way we used to get from getFirstHeight.

}
console.log('brandToPurse map', brandToPurse);
updatePurses();
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
};
return firstHeight;
};

Comment on lines 208 to 210
const followLatest = async () => {
console.log('followLatest()');
for await (const { value } of iterateEach(updateFollower)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const followLatest = async () => {
console.log('followLatest()');
for await (const { value } of iterateEach(updateFollower)) {
const followLatest = async firstHeight => {
console.log('followLatest()');
for await (const { value } of iterateEach(updateFollower, { height: firstHeight })) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent, thanks

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks overall good.

I would prefer more careful handoff between the current state and the { height: lastHeight } parameter of the update stream, but even just a comment to that effect would be fine.

@dckc
Copy link
Member

dckc commented Sep 30, 2022

Since this has on-chain changes, it needs to go on the release-pismo branch, one way or the other.

@turadg turadg force-pushed the 6184-wallet-current branch from 4d97f9c to 8e94f86 Compare September 30, 2022 15:37
@turadg turadg requested a review from dtribble as a code owner September 30, 2022 15:37
@turadg turadg changed the base branch from master to release-pismo September 30, 2022 15:37
@turadg turadg changed the base branch from release-pismo to master September 30, 2022 15:39
@turadg turadg force-pushed the 6184-wallet-current branch from 8e94f86 to fa3e4e6 Compare September 30, 2022 15:39
@turadg turadg force-pushed the 6184-wallet-current branch from fa3e4e6 to 4c6544b Compare September 30, 2022 15:40
@turadg
Copy link
Member Author

turadg commented Sep 30, 2022

Since this has on-chain changes, it needs to go on the release-pismo branch, one way or the other.

I tried to make this PR base of release-pismo but it has diverged enough from master that the rebase was somewhat cumbersome. I'll wait for this to get into master and then do a separate PR to get the commits into pismo.

@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Sep 30, 2022
@mergify mergify bot merged commit 32707c4 into master Sep 30, 2022
@mergify mergify bot deleted the 6184-wallet-current branch September 30, 2022 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants