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

Query bank balances #51

Merged
merged 8 commits into from
Jan 27, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/components/SmartWalletConnection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ const SmartWalletConnection = ({
makeFollower(`:published.${path}`, leader, {
unserializer: context.fromMyWallet,
});

const bridge = makeWalletBridgeFromFollowers(
{
chainId: keplrConnection.chainId,
Expand All @@ -184,6 +185,8 @@ const SmartWalletConnection = ({
makeFollower(`:beansOwing.${publicAddress}`, leader, {
unserializer: { unserialize: data => data },
}),
followPublished('agoricNames.vbankAsset'),
followPublished('agoricNames.brand'),
keplrConnection,
// @ts-expect-error xxx
backendError,
Expand Down
56 changes: 32 additions & 24 deletions src/service/Offers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
makeAsyncIterableFromNotifier,
} from '@agoric/notifier';
import { E } from '@endo/eventual-send';

import {
loadOffers as load,
removeOffer as remove,
Expand All @@ -17,11 +16,10 @@ import {

import type { SmartWalletKey } from '../store/Dapps';
import type { OfferSpec, OfferStatus } from '@agoric/smart-wallet/src/offers';
import { Marshal } from '@endo/marshal';

import type { Marshal } from '@endo/marshal';
import type { Notifier } from '@agoric/notifier/src/types';
import { Petname } from '@agoric/smart-wallet/src/types';
import { Brand } from '@agoric/ertp/src/types';
import type { Petname } from '@agoric/smart-wallet/src/types';
import type { Brand } from '@agoric/ertp/src/types';

export const getOfferService = (
smartWalletKey: SmartWalletKey,
Expand All @@ -41,33 +39,43 @@ export const getOfferService = (
id,
instanceHandle,
publicInvitationMaker,
proposalTemplate: { give, want },
proposalTemplate: { give: giveTemplate, want: wantTemplate },
} = offer;

const mapPursePetnamesToBrands = paymentProposals =>
Object.fromEntries(
const convertProposals = async paymentProposals => {
const entries = await Promise.all(
Copy link
Member

Choose a reason for hiding this comment

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

this is a little hard to follow. consider filtering and naming the value transformer.

const convertProposal = ({ pursePetname, value, amount: serializedAmount }) =>
  serializedAmount
              ? await E(boardIdMarshaller).unserialize(serializedAmount)
              : AmountMath.make(
                  pursePetnameToBrand.get(pursePetname),
                  BigInt(value);

Object.entries(paymentProposals).
filter(([_kw, { pursePetname, amount}]) => amount && pursePetnameToBrand.get(pursePetname))
.map(([kw, proposal]) => convertProposal);

Then it's a small step to using objectMap and deeplyFulfilled;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave this a shot but I got a little confused with how to use those utility functions and the code wasn't looking much simpler, I'd prefer to leave this as is.

Object.entries(paymentProposals).map(
// @ts-expect-error
([kw, { pursePetname, value }]) => {
const brand = pursePetnameToBrand.get(pursePetname);
if (!brand) {
async ([kw, { pursePetname, value, amount: serializedAmount }]) => {
/// XXX test e2e with dapp inter once feasible.
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the documenting. consider TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const amount = serializedAmount
? await E(boardIdMarshaller).unserialize(serializedAmount)
: { brand: pursePetnameToBrand.get(pursePetname), value };
Copy link
Member

Choose a reason for hiding this comment

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

we're cool with open-coding AmountMath.make()? maybe the notion of brand used here doesn't quite line up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you caught a bug, I wasn't converting value to BigInt. Just changed to use AmountMath and confirmed it works.


if (!(amount.brand && amount.value)) {
return [];
}
return [
kw,
{
brand,
value: BigInt(value),
},
];
return [kw, amount];
},
),
);
return Object.fromEntries(entries);
};

const instance = await E(boardIdMarshaller).unserialize(instanceHandle);
const {
slots: [instanceBoardId],
} = await E(boardIdMarshaller).serialize(instance);
const deconstructInstance = async () => {
const instance = await E(boardIdMarshaller).unserialize(instanceHandle);
const {
slots: [instanceBoardId],
} = await E(boardIdMarshaller).serialize(instance);
Copy link
Member

Choose a reason for hiding this comment

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

i had to remember there's no promise pipelining in this app so the two awaits are fine.

it wasn't obvious to me why you wanted to wrap these in a function. It looks like you want convertProposals to resolve in parallel to resolving the instance. Good idea. But I don't see why you want instanceBoardId to be available after this Promise.all.

consider,

    const [instance, give, want] = await Promise.all([
      E(boardIdMarshaller).unserialize(instanceHandle),
      convertProposals(giveTemplate),
      convertProposals(wantTemplate),
    ]);

and then just above the first use of instanceBoardId,

    const {
      slots: [instanceBoardId],
    } = await E(boardIdMarshaller).serialize(instance);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I'm not sure about promise pipelining in the rest of the app, but we await here so that the card doesn't show up before it has the data needed to render.


return { instance, instanceBoardId };
};

const [{ instance, instanceBoardId }, give, want] = await Promise.all([
deconstructInstance(),
convertProposals(giveTemplate),
convertProposals(wantTemplate),
]);

const offerForAction: OfferSpec = {
id,
Expand All @@ -77,8 +85,8 @@ export const getOfferService = (
publicInvitationMaker,
},
proposal: {
give: mapPursePetnamesToBrands(give),
want: mapPursePetnamesToBrands(want),
give,
want,
},
};

Expand Down
119 changes: 108 additions & 11 deletions src/util/WalletBackendAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { objectMap } from '@agoric/internal';
import {
makeAsyncIterableFromNotifier,
makeNotifierKit,
observeNotifier,
} from '@agoric/notifier';
import {
assertHasData,
Expand All @@ -29,8 +30,11 @@ import { KeplrUtils } from '../contexts/Provider.jsx';
import type { PurseInfo } from '@agoric/web-components/src/keplr-connection/fetchCurrent';
import { HttpEndpoint } from '@cosmjs/tendermint-rpc';
import type { ValueFollowerElement } from '@agoric/casting/src/types';
import { queryBankBalances } from './queryBankBalances';
import type { Coin } from '@cosmjs/stargate';

const newId = kind => `${kind}${Math.random()}`;
const POLL_INTERVAL_MS = 6000;

export type BackendSchema = {
actions: object;
Expand Down Expand Up @@ -108,6 +112,7 @@ export const makeBackendFromWalletBridge = (

const cancel = e => {
backendUpdater.fail(e);
walletBridge.cleanup();
};

return { backendIt, cancel };
Expand All @@ -120,13 +125,20 @@ export const makeWalletBridgeFromFollowers = (
currentFollower: ValueFollower<CurrentWalletRecord>,
updateFollower: ValueFollower<UpdateRecord>,
beansOwingFollower: ValueFollower<string>,
vbankAssetsFollower: ValueFollower<unknown>,
agoricBrandsFollower: ValueFollower<unknown>,
Copy link
Member

Choose a reason for hiding this comment

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

please include the types. if they can't be imported just write out what this code is assuming.

Copy link
Member

Choose a reason for hiding this comment

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

also this is getting very long for a positional arguments list. very much worth turning into a named options argument, though not necessarily this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added types here and a bit below.

Copy link
Member

Choose a reason for hiding this comment

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

I was going to say something about the unknown type, but if it typechecks (without casts), that means the code doesn't care what the type is, right? i.e. the code is assuming nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but we were getting away with it because iterateLatest spits out any which we avoid doing explicitly.

keplrConnection: KeplrUtils,
errorHandler = e => {
// Make an unhandled rejection.
throw e;
},
firstCallback: () => void | undefined = () => {},
) => {
let isHalted = false;
let isBankLoaded = false;
let isSmartWalletLoaded = false;
let isOfferServiceStarted = false;

const notifiers = {
getPursesNotifier: 'purses',
getContactsNotifier: 'contacts',
Expand All @@ -142,14 +154,12 @@ export const makeWalletBridgeFromFollowers = (
]),
);

const { notifier: beansOwingNotifier, updater: beansOwingUpdater } =
makeNotifierKit<Number | null>(null);

// We assume just one cosmos purse per brand.
const brandToPurse = new Map<Brand, PurseInfo>();
const pursePetnameToBrand = new Map<Petname, Brand>();

const updatePurses = () => {
console.debug('brandToPurse map', brandToPurse);
const purses = [] as PurseInfo[];
for (const [brand, purse] of brandToPurse.entries()) {
if (purse.currentAmount && purse.brandPetname) {
Expand All @@ -159,6 +169,13 @@ export const makeWalletBridgeFromFollowers = (
}
}
notifierKits.purses.updater.updateState(harden(purses));

// Make sure the offer service has all purses from both bank and smart
// wallet chainstorage.
if (!isOfferServiceStarted && isBankLoaded && isSmartWalletLoaded) {
isOfferServiceStarted = true;
offerService.start(pursePetnameToBrand);
}
};

const signSpendAction = async (data: string) => {
Expand Down Expand Up @@ -188,15 +205,79 @@ export const makeWalletBridgeFromFollowers = (
marshaller,
);

const { notifier: beansOwingNotifier, updater: beansOwingUpdater } =
makeNotifierKit<Number | null>(null);

const watchBeansOwing = async () => {
for await (const { value } of iterateLatest(beansOwingFollower)) {
if (isHalted) return;
beansOwingUpdater.updateState(Number(value));
}
};

// Reads purses from the cosmos bank module. These are not necessarily real
Copy link
Member

Choose a reason for hiding this comment

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

consider "Infer purses from Cosmos bank module balances" since, as you say, purses can't be read from Cosmos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// "purses" in the smart wallet contract, because those are lazily
// instantiated, but we still need to show the balances.
const watchChainBalances = () => {
let vbankAssets;
let bank;

const possiblyUpdateBankPurses = () => {
if (!vbankAssets || !bank) return;

const bankMap = new Map<string, string>(
bank.map(({ denom, amount }) => [denom, amount]),
);

vbankAssets.forEach(([denom, info]) => {
const amount = bankMap.get(denom) ?? 0n;
Copy link
Contributor Author

@samsiegart samsiegart Jan 26, 2023

Choose a reason for hiding this comment

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

If an asset exists in the vbank but not in the user's account we want to show it as a purse with 0 balance so they can click the eventual "deposit" button for ibc transfer.

Copy link
Member

Choose a reason for hiding this comment

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

Very helpful note. Please include in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const purseInfo: PurseInfo = {
brand: info.brand,
currentAmount: AmountMath.make(info.brand, BigInt(amount)),
brandPetname: info.issuerName,
pursePetname: info.issuerName,
displayInfo: info.displayInfo,
};
brandToPurse.set(info.brand, purseInfo);
});

isBankLoaded = true;
updatePurses();
};

const watchBank = async () => {
if (isHalted) return;
const balances = await queryBankBalances(keplrConnection.address, rpc);
bank = balances;
Copy link
Member

Choose a reason for hiding this comment

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

why the const?

Suggested change
const balances = await queryBankBalances(keplrConnection.address, rpc);
bank = balances;
bank = await queryBankBalances(keplrConnection.address, rpc);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, done

possiblyUpdateBankPurses();
setTimeout(watchBank, POLL_INTERVAL_MS);
};

const watchVbankAssets = async () => {
for await (const { value } of iterateLatest(vbankAssetsFollower)) {
if (isHalted) return;
vbankAssets = value;
possiblyUpdateBankPurses();
}
};

void watchVbankAssets();
void watchBank();
};

const fetchAgoricBrands = async () => {
for await (const { value } of iterateLatest(agoricBrandsFollower)) {
if (value) {
Copy link
Member

Choose a reason for hiding this comment

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

when would the iterator return an object without a value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was afraid it might have an initial value of null or something, but that's more a concern for notifiers, and it would throw anyway in that case. Just removed the conditional

return new Map(value.map(([k, v]) => [v, k]));
Copy link
Member

Choose a reason for hiding this comment

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

This took me some time to read through without type info. I think it's making a map with the keys/values inverted. Consider https://lodash.com/docs/4.17.15#invert to make that clear, or a code comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added types and left a comment

}
}
};

const fetchCurrent = async () => {
await assertHasData(currentFollower);
void watchBeansOwing();
watchChainBalances();

const latestIterable = await E(currentFollower).getLatestIterable();
const iterator = latestIterable[Symbol.asyncIterator]();
const latest = await iterator.next();
Expand All @@ -210,13 +291,29 @@ export const makeWalletBridgeFromFollowers = (
}
const currentEl: ValueFollowerElement<CurrentWalletRecord> = latest.value;
const wallet = currentEl.value;
console.log('wallet current', wallet);
console.debug('wallet current', wallet);

const agoricBrands = await fetchAgoricBrands();
assert(agoricBrands, 'Failed to fetch agoric brands');

for (const purse of wallet.purses) {
console.debug('registering purse', purse);
const brandDescriptor = wallet.brands.find(
bd => purse.brand === bd.brand,
);
assert(brandDescriptor, `missing descriptor for brand ${purse.brand}`);
// We only care about the zoe invite purse, which has asset kind 'set'.
// Non 'set' amounts need to be fetched from vbank to know their
// decimalPlaces.
//
// If we have add non 'set' amount purses that aren't in the vbank, it's
// not currently possible to read their decimalPlaces, so this code
// will need updating.
Copy link
Member

Choose a reason for hiding this comment

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

This documentation is good.

Ideally we'd import some invitationIssuerAssetKind constant from @agoric/zoe/something.

I leave it to @turadg to judge the cost-effectiveness of that.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know of any runtime features of an invitation issuer that distinguishes it from any other Set kind issuer.

You could make this "set"-ness check more apparent using @agoric/patterns (soon to be in Endo): matches(purse.balances.value, SetValueShape).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like @agoric/patterns doesn't exist yet either, but that would be cool.

Copy link
Member

Choose a reason for hiding this comment

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

Oops I meant '@agoric/store'. But yeah let's wait for it to be in Endo

if (
!Array.isArray(purse.balance.value) ||
!agoricBrands.has(purse.brand)
) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

consider distinguishing these conditions:

if (!Array.isArray(purse.balance.value)) {
  console.debug('skipping non-set amount', purse.balance.value);
}
if (!agoricBrands.has(purse.brand)) {
  console.debug('skipping unknown brand', purse.brand);

Likely to pay off debugging someday. And in the meantime it's code documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const brandDescriptor = {
petname: agoricBrands.get(purse.brand),
displayInfo: { assetKind: 'set' },
};
const purseInfo: PurseInfo = {
brand: purse.brand,
currentAmount: purse.balance,
Expand All @@ -226,9 +323,8 @@ export const makeWalletBridgeFromFollowers = (
};
brandToPurse.set(purse.brand, purseInfo);
}
console.debug('brandToPurse map', brandToPurse);
isSmartWalletLoaded = true;
updatePurses();
offerService.start(pursePetnameToBrand);
return currentEl.blockHeight;
};

Expand Down Expand Up @@ -340,6 +436,7 @@ export const makeWalletBridgeFromFollowers = (
makeEmptyPurse,
addContact,
addIssuer,
cleanup: () => (isHalted = true),
});

return walletBridge;
Expand Down
21 changes: 21 additions & 0 deletions src/util/queryBankBalances.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { QueryClient, createProtobufRpcClient } from '@cosmjs/stargate';
import { Tendermint34Client } from '@cosmjs/tendermint-rpc';
import { QueryClientImpl } from 'cosmjs-types/cosmos/bank/v1beta1/query';
import type { Coin } from '@cosmjs/stargate';
import type { HttpEndpoint } from '@cosmjs/tendermint-rpc';

export const queryBankBalances = async (
address: string,
rpc: HttpEndpoint,
): Promise<Coin[]> => {
const tendermint = await Tendermint34Client.connect(rpc);
Copy link
Member

Choose a reason for hiding this comment

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

ambient authority. hm.

No modules should export powerful objects (for example, objects that close over fs or process.env). -- https:/Agoric/agoric-sdk/wiki/OCap-Discipline

I suppose we have agreed on an exception to that for UI code? I'd like to see that in the README or the like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, I don't know if I have a clear way to explain the philosophy as it pertains to UI code at the moment though.

Copy link
Member

Choose a reason for hiding this comment

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

I think we have agreed agreed on an exception to that for UI code and I agree it would be valuable to have in the README of this repo to refer to. Not a blocker imo

Copy link
Member

Choose a reason for hiding this comment

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

agreed, not a blocker.

meanwhile, I realize pointing to the wiki doesn't automatically make a link the other way, where citing an issue does: Agoric/agoric-sdk#2160

const queryClient = new QueryClient(tendermint);
const rpcClient = createProtobufRpcClient(queryClient);
const bankQueryService = new QueryClientImpl(rpcClient);

const { balances } = await bankQueryService.AllBalances({
Copy link
Member

Choose a reason for hiding this comment

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

scaling consideration: How often do we do this? I gather from @arirubinstein that we're supposed to make no more than 2 RPC queries per second.

I gather @michaelfig plans to provide rate limiting in @agoric/casting (IOU an issue #) Do we have an issue about migrating this code to use it? Cite it from a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea my hope is this could unblock us for now and we can swap in the improved casting stuff later, just added a comment here.

Copy link
Member

Choose a reason for hiding this comment

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

address,
});

return balances;
};
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -12164,7 +12164,7 @@ rollup@^2.43.1:
optionalDependencies:
fsevents "~2.3.2"

rollup@endojs/endo#rollup-2.7.1-patch-1, "rollup@github:endojs/endo#rollup-2.7.1-patch-1":
rollup@endojs/endo#rollup-2.7.1-patch-1:
Copy link
Member

Choose a reason for hiding this comment

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

curious.

version "2.70.1-endo.1"
resolved "https://codeload.github.com/endojs/endo/tar.gz/54060e784a4dbe77b6692f17344f4d84a198530d"
optionalDependencies:
Expand Down