feat: cli tool to setup the sdk and circle console #58
Conversation
krzysu
force-pushed
the
circle-webapp-2
branch
from
bef2667 to
b67bf5d
Compare
krzysu
force-pushed
the
sdk-setup-cli
branch
3 times, most recently
from
9f04775 to
cb3cab0
Compare
krzysu
requested review from
jdevcs,
avkos,
danforbes and
Muhammad-Altabba
and removed request for
jdevcs
| return false; | ||
| } catch (error) { | ||
| console.error('Error checking existing configuration:', error); | ||
| return false; |
There was a problem hiding this comment.
Shouldn't this throw and propagate the error?
There was a problem hiding this comment.
This is simply a check to verify if the file exists and contains the expected environment variables. It throws an error if the file doesn’t exist, which is fine for detecting the file’s presence. While reviewing this, I discovered another issue: the environment variable name was incorrect (CIRCLE_APP_ID instead of CIRCLE_API_KEY). I’ve fixed that.
In summary, this error doesn’t need to be propagated since its sole purpose is to check for the file’s existence.
packages/circle-sdk-setup/src/cli.ts
Outdated
| async initializeSdk(apiKey: string, secret: string): Promise<CircleSdk> { | ||
| console.log('Initializing Circle SDK...'); | ||
| return new CircleSdk(apiKey, secret); | ||
| } |
There was a problem hiding this comment.
Does this need to be async and return a Promise? Is this method even really needed?
There was a problem hiding this comment.
for context, that was my late night experiment with using AI agent to build something useful and I thought simple CLI tool is a good task for that 😄 . So I wasn't very strict with the results. I'm cleaning this up now, thanks for the review!
packages/circle-sdk-setup/src/cli.ts
Outdated
| let envContent = ''; | ||
| if (fs.existsSync(this.options.envPath)) { | ||
| envContent = fs.readFileSync(this.options.envPath, 'utf-8'); | ||
| // Remove existing Circle config if any |
There was a problem hiding this comment.
Is that what we'd want to do in this situation? I guess this goes along with the comment I left regarding the checkExistingConfig function.
There was a problem hiding this comment.
We should avoid removing anything from local user settings. Instead, notify the user that the configuration failed and suggest checking the Circle console. I spent a significant amount of time investigating whether Circle configuration can be removed, but found no option to do so. The only available actions are rotating keys or resetting the cipher, both of which are not possible via the API.
| @@ -2,8 +2,15 @@ | |||
| "name": "web3-circle-sdk", | |||
| "version": "0.1.0", | |||
| "description": "Web3 Circle SDK", | |||
| "type": "module", | |||
There was a problem hiding this comment.
@avkos can we have the sdk build and export setup for esm and cjs done asap (in main branch) as that impacts consuming it by other packages? I'm using only esm and I think we should aim to make ESM the default and commonjs as a fallback for legacy use. what do you think? cc @Muhammad-Altabba @jdevcs
krzysu
force-pushed
the
sdk-setup-cli
branch
from
08d4c76 to
e1eec90
Compare
|
@danforbes Alright, I think this PR is ready. I’ve tested detecting the .env file and writing a new one. The only step I couldn’t test is registerCiphertext and storing the recovery file (with the recovery content), as my Circle account is already set up. |
krzysu
force-pushed
the
sdk-setup-cli
branch
from
e1eec90 to
2c9f694
Compare
If somebody hasn't configured their circle console yet, please try this cli tool with