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

Wallet UI services #5949

Merged
merged 8 commits into from
Aug 15, 2022
Merged

Wallet UI services #5949

merged 8 commits into from
Aug 15, 2022

Conversation

samsiegart
Copy link
Contributor

refs: #5354

@samsiegart samsiegart requested review from turadg and dckc August 12, 2022 23:20
@samsiegart
Copy link
Contributor Author

@dckc I'd like your eyes specifically on the changes in marshal-context.js. Without those we get bad board slot unknown:1 when unserializing a contact in the ui.

@turadg For general js/react code review

@samsiegart samsiegart marked this pull request as ready for review August 12, 2022 23:28
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

No blockers. I assume you've tested manually

packages/wallet/api/src/marshal-contexts.js Show resolved Hide resolved
Comment on lines 67 to 72
try {
await E(offer.actions).accept();
} catch (e) {
setPendingOffers({ offerId: id, isPending: false });
console.error('Failed to accept offer', e);
}
Copy link
Member

Choose a reason for hiding this comment

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

with this try, returned promise will now resolve even if accept fails. I don't know if anything is actually paying attention to the result (there's no return type) but I think it would be safer to keep this function sync and return the failed promise.

Suggested change
try {
await E(offer.actions).accept();
} catch (e) {
setPendingOffers({ offerId: id, isPending: false });
console.error('Failed to accept offer', e);
}
return E(offer.actions).accept().catch(e => {
setPendingOffers({ offerId: id, isPending: false });
console.error('Failed to accept offer', e);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's better, done thanks

give = proposalTemplate.give ?? {};
want = proposalTemplate.want ?? {};
args = proposalTemplate.arguments;
}
Copy link
Member

Choose a reason for hiding this comment

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

is it intended that ! proposalForDisplay && ! proposalTemplate result in give = {} etc? Consider including an explanation.

Suggested change
}
} else {
// leave default values
}

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, added an explanation

@@ -0,0 +1,44 @@
import { makeNotifierKit } from '@agoric/notifier';

export const getIssuerService = signSpendAction => {
Copy link
Member

Choose a reason for hiding this comment

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

consider typing

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


export const getIssuerService = signSpendAction => {
const suggestions = new Map();
const { notifier, updater } = makeNotifierKit();
Copy link
Member

Choose a reason for hiding this comment

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

not worth changing right now but for the future consider makePublishKit

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 TODO

@samsiegart samsiegart force-pushed the wallet-ui-services branch 2 times, most recently from 6607b6f to a1e470b Compare August 14, 2022 03:00
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.

looks good.

In particular, test coverage for marshal-contexts.js is still 100%

/** @type {IdTable<number, Payment>} */
/** @type {IdTable<number, unknown>} */
Copy link
Member

Choose a reason for hiding this comment

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

oops! Thanks for fixing that.

/** @type {IdTable<number, PurseActions>} */
/** @type {IdTable<number, Purse>} */
Copy link
Member

Choose a reason for hiding this comment

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

yes, I was probably getting ahead of myself with PurseActions.

@@ -1205,7 +1205,7 @@ test('lib-wallet performAction suggestIssuer', async t => {
type: 'suggestIssuer',
data: { petname: 'bucksIssuer', boardId: bucksIssuerBoardId },
});
const done = await wallet.performAction({ action });
const done = await wallet.performAction({ spendAction: action });
Copy link
Member

Choose a reason for hiding this comment

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

suggestIssuer is a spendAction? I suppose that's correct but just a sort of de-optimization.

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 haven't tried getting a non-spend action end-to-end working, I assumed that was still needing design but perhaps this can be the first one we try with. We can revisit.

@@ -241,17 +241,19 @@ const defaultMakePresence = iface => {
*/
export const makeImportContext = (makePresence = defaultMakePresence) => {
const walletObjects = {
/** @type {IdTable<number, PurseActions>} */
Copy link
Member

Choose a reason for hiding this comment

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

ouch... why did we have to take that out? Can we make it IdTable<number, unknown>? or even IdTable<number, *>? Using * (aka any) should include a justification 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 it back with unknown

@samsiegart samsiegart added the automerge:squash Automatically squash merge label Aug 15, 2022
@mergify mergify bot merged commit a457f75 into master Aug 15, 2022
@mergify mergify bot deleted the wallet-ui-services branch August 15, 2022 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants