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

feat: support non-vbank purses #37

Merged
merged 2 commits into from
Aug 25, 2023
Merged

feat: support non-vbank purses #37

merged 2 commits into from
Aug 25, 2023

Conversation

samsiegart
Copy link
Contributor

refs Agoric/wallet-app#116

Testing

Tested with https://github.com/samsiegart/simple-game (using yarn link @agoric/rpc and yarn link @agoric/web-components on this branch locally). Running a local chain against Agoric/agoric-sdk#8147

Also: chainapsis/keplr-wallet@master...samsiegart:keplr-wallet:agoric-asset-support

Capture

res(val);
stop();
Copy link
Member

Choose a reason for hiding this comment

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

consider: Conceptually, shouldn't it stop as soon as it got the value? If it were async you could imagine that by the time the resolve was done another value would have been watched. I don't see how that would be possible in this synchronous function but it still seems more appropriate to stop immediately upon receiving the one 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.

Sure, swapped them around. Yea it doesn't matter in without an await.

@@ -201,10 +201,30 @@ export const makeAgoricChainStorageWatcher = (
return () => stopWatching(pathKey, subscriber as Subscriber<unknown>);
};

const queryOnce = <T>(path: [AgoricChainStoragePathKind, string]) =>
Copy link
Member

Choose a reason for hiding this comment

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

remark: nice use of generic. eventually I'd like agoric-sdk to export a schema of types for each storage path so that the type can be inferred from the path string.

});

const queryBoardAux = <T>(boardObjects: unknown[]) => {
const boardIds = boardObjects.map(o => marshaller.toCapData(o).slots[0]);
Copy link
Member

Choose a reason for hiding this comment

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

important: the callback taking slot[0] is making some assumptions. please extract it into a function with some documentation on why that assumption is safe

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

@@ -110,6 +110,29 @@ describe('makeAgoricChainStorageWatcher', () => {
expect(await value).toEqual(expected);
});

it('can do single queries', async () => {
const expected = 126560000000;
const path = 'published.vitest.unserializedValue';
Copy link
Member

Choose a reason for hiding this comment

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

Important: is published necessary? if not remove it and if so explain in a 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.

No just to illustrate, removed

Comment on lines 131 to 133
let agoricBrands;
let nonBankPurses;
let brandToBoardAux;
Copy link
Member

Choose a reason for hiding this comment

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

important: the is a lot of state in this large scope. it's hard to keep track of the possible states and types.

please reduce the access to these vars to only the functions that need them. a block may suffice.

please also annotate their types.

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

@samsiegart samsiegart enabled auto-merge (squash) August 25, 2023 17:45
@samsiegart samsiegart merged commit e2665de into main Aug 25, 2023
1 check passed
@samsiegart samsiegart deleted the non-vbank-purses branch August 25, 2023 17:46
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

Successfully merging this pull request may close these issues.

2 participants