-
Notifications
You must be signed in to change notification settings - Fork 22
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(client): optional account recovery #1546
Conversation
Signed-off-by: Felipe Forbeck <felipe.forbeck@gmail.com>
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 approach is fine, except I have one nit around using an account object rather than an email.
Also we should submit a PR to github.com/storacha-network/docs to update the docs for the JS client.
The last commit introduces a new test case to ensure that when Alice creates a space, her account is correctly set as the recovery account. The test verifies that Alice retains access to the space from another device by checking the space's accessibility. In the If that looks good to you I will update the 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.
Yea, this is obviously not ideal to have all this logic in creating a space, on the otherhand, with the chain of dependencies there's no way to seperate it. And it will make for a much shorter setup tutorial, which will be quite nice. We should definitely update docs for this.
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.
LGTM
f7dadb8
to
a01012c
Compare
9531e3b
to
eed9693
Compare
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 great but please could you try to keep non essential changes to a minimum? There's a lot of code formatting change in here that makes it harder for me to review. Just submit a separate PR in the future for code formatting changes 🙏.
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>
Yeah, my bad. I will revert these formatting changes to simplify this PR. |
The CLI should remain unaffected by this change because the In the future, we could create a separate PR to simplify the CLI code by leveraging the new version of the |
60dcbc9
to
f38507b
Compare
f38507b
to
0e6d683
Compare
@alanshaw |
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.
LGTM
|
||
```js | ||
// wait for payment plan to be selected | ||
while (true) { | ||
const res = await account.plan.get() | ||
if (res.ok) break | ||
console.log('Waiting for payment plan to be selected...') | ||
await new Promise(resolve => setTimeout(resolve, 1000)) | ||
await new Promise((resolve) => setTimeout(resolve, 1000)) | ||
} |
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.
It would be rad if this was a method of the client? Would you be up for adding something like that quickly?
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.
Good idea. I will add that.
packages/w3up-client/README.md
Outdated
await new Promise((resolve) => setTimeout(resolve, 1000)) | ||
} | ||
// Wait for a payment plan with a 1-second polling interval and 60-second timeout | ||
await client.waitForPaymentPlan(account, 1000, 60000) |
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.
🙏 can we land as a separate PR? Apologies - I should have asked...
I think it might be better for this to be an instance method of AccountPlan
rather than the top level? WDYT?
await account.plan.wait()
In the docs I think it's simplier to just use the defaults (i.e. don't specify the interval/timeout).
packages/w3up-client/src/client.js
Outdated
* @param {Account.Account} account - The account object to check the payment plan for. | ||
* @param {number} [interval=1000] - The polling interval in milliseconds (default is 1000ms). | ||
* @param {number} [timeout=180000] - The maximum time to wait in milliseconds before throwing a timeout error (default is 3 minutes). | ||
* @returns {Promise<void>} - Resolves once a payment plan is selected within the timeout. |
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 would return the plan information when it is available.
packages/w3up-client/src/client.js
Outdated
* at a specified interval until a valid plan is selected or the timeout is reached. | ||
* | ||
* @param {Account.Account} account - The account object to check the payment plan for. | ||
* @param {number} [interval=1000] - The polling interval in milliseconds (default is 1000ms). |
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'd pass an options object with these two properties.
packages/w3up-client/src/client.js
Outdated
* | ||
* @param {Account.Account} account - The account object to check the payment plan for. | ||
* @param {number} [interval=1000] - The polling interval in milliseconds (default is 1000ms). | ||
* @param {number} [timeout=180000] - The maximum time to wait in milliseconds before throwing a timeout error (default is 3 minutes). |
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.
Might be better to pass an AbortSignal
instead so that if folks are using this in a UI they can have a cancel button. Another reason is that it could take longer than 3 minutes, considering you have to log into your email, click the link, add credit card details etc.
message: 'Timeout: Payment plan selection took too long.', | ||
}) | ||
}, | ||
}, |
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.
❤️
7d268e5
to
a46d59d
Compare
🤖 I have created a release *beep* *boop* --- ## [16.1.0](w3up-client-v16.0.0...w3up-client-v16.1.0) (2024-09-16) ### Features * **client:** optional account recovery ([#1546](#1546)) ([ea02adb](ea02adb)) * wait for plan selection ([#1547](#1547)) ([7fdee77](7fdee77)) ### Other Changes * Add `pnpm dev` to watch-build all packages ([#1533](#1533)) ([07970ef](07970ef)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
### Changes The `createSpace` function in `client.js` has been enhanced to streamline the space creation process. The new workflow now automatically handles the following steps: - Provisions the space. - Saves the space. - Creates a recovery account. - Delegates access to the recovery account. This update removes the need for manual steps, ensuring the recovery account is set up as part of the space creation process. > **Note:** This PR should only be merged after [storacha/w3up#1546](storacha/w3up#1546) has been merged and released.
Issue storacha/project-tracking#124
Current Workflow (CLI)
w3cli
ProjectThe
space.create(name, options)
function inspace.js
allows the user to pass the following options:false
(default)false
(default)Email | false
string | false
If the
account
attribute is provided, the following steps are triggered:space.createdRecovery
is called.client.capabilities.access.delegate
is executed.This process occurs during space creation, not during provisioning.
Provisioning only happens when the
customer
attribute is provided in theoptions
argument.Issue (JS Client)
w3up
ProjectThe
client.createSpace(name)
function in thew3up
project currently does not accept any options, which means the recovery account must be created manually after space creation and provisioning. This introduces a risk of forgetting the manual step and potentially losing access to the space.Expected Outcome
We propose a small modification to the
client.createSpace(name)
function to support optional parameters, allowing clients to pass anaccount
directly, automating the recovery setup.Solution:
client.createSpace(name, { account })
to include theaccount
in the creation process.Benefits:
account
parameter is optional, keeping calls likeclient.createSpace(name)
intact.account
when callingcreateSpace
, the recovery account setup is handled automatically during space creation.account
attribute can be made required to enforce stricter recovery account management.