-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
Deploying with Cloudflare Pages
|
); | ||
|
||
vbankAssets.forEach(([denom, info]) => { | ||
const amount = bankMap.get(denom) ?? 0n; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks plausible.
I'd like to see test coverage for this feature, as usual. Skipping it for now is OK with me if it's OK with @turadg .
src/service/Offers.ts
Outdated
/// XXX test e2e with dapp inter once feasible. | ||
const amount = serializedAmount | ||
? await E(boardIdMarshaller).unserialize(serializedAmount) | ||
: { brand: pursePetnameToBrand.get(pursePetname), value }; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
address: string, | ||
rpc: HttpEndpoint, | ||
): Promise<Coin[]> => { | ||
const tendermint = await Tendermint34Client.connect(rpc); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 rpcClient = createProtobufRpcClient(queryClient); | ||
const bankQueryService = new QueryClientImpl(rpcClient); | ||
|
||
const { balances } = await bankQueryService.AllBalances({ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/util/WalletBackendAdapter.ts
Outdated
// 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
const mapPursePetnamesToBrands = paymentProposals => | ||
Object.fromEntries( | ||
const convertProposals = async paymentProposals => { | ||
const entries = await Promise.all( |
There was a problem hiding this comment.
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
;
There was a problem hiding this comment.
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.
src/service/Offers.ts
Outdated
}, | ||
]; | ||
|
||
/// XXX test e2e with dapp inter once feasible. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/service/Offers.ts
Outdated
const instance = await E(boardIdMarshaller).unserialize(instanceHandle); | ||
const { | ||
slots: [instanceBoardId], | ||
} = await E(boardIdMarshaller).serialize(instance); |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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.
src/util/WalletBackendAdapter.ts
Outdated
beansOwingUpdater.updateState(Number(value)); | ||
} | ||
}; | ||
|
||
// Reads purses from the cosmos bank module. These are not necessarily real |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
); | ||
|
||
vbankAssets.forEach(([denom, info]) => { | ||
const amount = bankMap.get(denom) ?? 0n; |
There was a problem hiding this comment.
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.
src/util/WalletBackendAdapter.ts
Outdated
// 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. |
There was a problem hiding this comment.
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)
.
src/util/WalletBackendAdapter.ts
Outdated
if ( | ||
!Array.isArray(purse.balance.value) || | ||
!agoricBrands.has(purse.brand) | ||
) { | ||
continue; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/util/queryBankBalances.ts
Outdated
import type { Coin } from '@cosmjs/stargate'; | ||
import type { HttpEndpoint } from '@cosmjs/tendermint-rpc'; | ||
|
||
// XXX use casting support when possible for load balancing, batching, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a ticket to track when it's possible? I think we should. Consider noting it here with an UNTIL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the ticket yet, changed to UNTIL
address: string, | ||
rpc: HttpEndpoint, | ||
): Promise<Coin[]> => { | ||
const tendermint = await Tendermint34Client.connect(rpc); |
There was a problem hiding this comment.
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
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
refs #49
We still need to update dapp-inter to do the same, then I can test e2e with amounts in offer proposals rather than pursepetname+value