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

4484 multitenant smart wallet #5721

Merged
merged 2 commits into from
Jul 7, 2022
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Jul 5, 2022

refs: #4484

Description

Part of #4484. This doesn't do everything described in that ticket so I'm leaving it to the ticket author to decide what's left, if anything.

Security Considerations

Shared contract amongst all on-chain wallets. Potential DoS attack.

Documentation Considerations

--

Testing Considerations

New tests

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.

It would look better with tests for multiple addresses; I leave that up to you.

*/
export const provideAsync = async (mapStore, key, makeValue) => {
if (!mapStore.has(key)) {
mapStore.init(key, await makeValue(key));
Copy link
Member

Choose a reason for hiding this comment

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

this goes against No Nested Await. Is there any particular justification?

Copy link
Member Author

@turadg turadg Jul 7, 2022

Choose a reason for hiding this comment

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

Nope. Refactored with 6001a6d.

If you're not confident in that, it'll get cleaned up in #5728

Copy link
Member

Choose a reason for hiding this comment

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

The introduction of the pending Map doesn't seem equivalent to this. If it's essential, that seems to mean the nested await form was buggy. Anyway... it looks workable, and I'm content that #5728 is where we deal with it definitively.

Copy link
Member Author

Choose a reason for hiding this comment

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

The introduction of the pending Map doesn't seem equivalent to this

Right, I wasn't clear. The nested await went away as a side effect of the refactoring to makeAtomicProvider. The need for that function was because of a race condition I encountered when testing multiple wallets. Thanks for suggesting it!

},
}) => {
const STORAGE_PATH = 'wallet';

const storageNode = await getChildNode(chainStorage, STORAGE_PATH);
const marshaller = E(board).getPublishingMarshaller();
const { creatorFacet } = await E(zoe).startInstance(
Copy link
Member

Choose a reason for hiding this comment

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

FYI, dropping the admin facet here is not great; but we have plans elsewhere to address it: #4548 (comment)

return makeChainStorageRoot(toStorage, 'swingset', 'mockChainStorageRoot');
};

const mockAddress1 = 'mockAddress1';
Copy link
Member

Choose a reason for hiding this comment

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

how about testing 2 or 3 addresses / wallets?

@turadg turadg force-pushed the 5356-smart-wallet-contract branch 2 times, most recently from d217901 to 80f642b Compare July 6, 2022 21:26
@turadg turadg force-pushed the 4484-multitenant-smart-wallet branch from ae2027b to e0309aa Compare July 6, 2022 21:27
Base automatically changed from 5356-smart-wallet-contract to master July 6, 2022 23:28
@turadg turadg force-pushed the 4484-multitenant-smart-wallet branch from e0309aa to 57eac62 Compare July 7, 2022 15:24
@turadg turadg marked this pull request as ready for review July 7, 2022 15:27
@turadg turadg requested a review from dckc July 7, 2022 15:28
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.

Part of #4484. This doesn't do everything described in that ticket ...

really? ah. indeed... virtualization / durability.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Jul 7, 2022
@mergify mergify bot merged commit 9d42a8e into master Jul 7, 2022
@mergify mergify bot deleted the 4484-multitenant-smart-wallet branch July 7, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants