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

Initial implementation of the Armory SDK #241

Merged
merged 16 commits into from
May 7, 2024
Merged

Conversation

Ptroger
Copy link
Contributor

@Ptroger Ptroger commented May 2, 2024

In order to make basic-flow example work in your local, you'll need to:

  • An engine, a vault and Devtool setuped together and running.
  • A USER_PRIVATE_KEY in .env, (hex starting with 0x)

You need to replace in examples/basic/src/index.ts

  const config = createArmoryConfig({
    authClientId: '915ed686-ddcd-4018-95f9-69405d2dd740',
    authHost: 'http://localhost:3010',
    authSecret: 'fb46a34690a49c41c6093ada79a14fbefe7e158162ee62da1ebcce47ec893c63d3d015f06de8139e29eb',
    vaultClientId: '4edb675c-3df1-405b-b495-d93229e57b09',
    vaultHost: 'http://localhost:3011',
    vaultSecret: '5014c1a89e2224642ba05217d82cb71d6d07e9edf6d48b57a1c6ac8cce5a8e4b33d4c114f6332824b6ee',
    signer: privateKeyToJwk(UNSAFE_PRIVATE_KEY.Alice)
  })

these values with the correct one you are using.

Also, depending on the EOA you are using to send a transaction (the one you gave the private key in env), you'll need to adjust nonce field of the transactionRequest.

    action: 'signTransaction',
    transactionRequest: {
      from: vaultWalletAddress,
      chainId: 137,
      gas: BigInt(22000),
      to: anotherAddress,
      value: '0x111',
      nonce: 10 <----- This nonce
    },
    resourceId: resourceId(walletId),
    nonce: v4()
  }

This is happening live on polygon, so take care the address you send from and to. You'll need fund to make it work.

Copy link
Collaborator

@wcalderipe wcalderipe left a comment

Choose a reason for hiding this comment

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

Good job.

I left some comments to kick a conversation.

apps/vault/src/shared/guard/authorization.guard.ts Outdated Show resolved Hide resolved
@@ -31,6 +32,7 @@ export class ImportController {
body.walletId
)
} else if (body.privateKey) {
console.log('un-encripted private key')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.log('un-encripted private key')

@@ -0,0 +1,80 @@
{
"name": "basic-flow",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding an example project.

I'm thinking about a better name that describes what it's properly. What do you think armory-sdk-nodejs? I'm adding the runtime in the name of the example because it doesn't solve the problem of using it on the browser.

Remember that the directory structure must reflect the project name (e.g. examples/armory-sdk-nodejs/project.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that is actually a very good naming. I tried multiple ones, and gave up with this one as nothing was satisfying.

I did 'basic' because I'm expecting other examples will complexify with time, and this example felt like the most straightforward way to use our system

const main = async () => {
const anotherAddress = '0x3f843E606C79312718477F9bC020F3fC5b7264C2'.toLowerCase() as Hex

const config = createArmoryConfig({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have you changed the naming?

Suggested change
const config = createArmoryConfig({
const config = createArmoryClient({

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'm expecting that we will have an armory client later, that will be instantiated and extends viem clients. It will have other capabilities.
Also, as we integrate with other tools in web3, when they have a client its almost always something that was instantiated with a transport and that has actual broadcasting capabilities. Let's stay on that path, and keep this naming for when we have something with these capabilities

Comment on lines 94 to 98
/**
* @param config
* @param input
* @returns SignatureResponse
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the value of a docstring without any text that's exactly like the type signature of the function?

}
}

export const buildGnapScopedHeaders = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the goal of this function in the future?

I'm assuming you have a goal because the usage of the word "scope" means something else in an authz flow. Therefore, the implementation and name today are misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'Scoped' here, means that the user that is asking for the transaction, needs to also sign this header. So that the valid AccessToken, that was generated by the engine, will only be valid with this header signed by the user that also wants to perform the transaction.

So yes, it's not about action scopes, but about vault authorization.

I used 'scope' because we kept talking about 'user scoped headers'. Do you have a better idea on the naming ?

Comment on lines +137 to +138
'x-client-id': config.vaultClientId,
'x-client-secret': config.vaultSecret
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hint: you can put these into constants and re-use them.

htm: 'POST',
uri,
created: new Date().getTime(),
ath: hexToBase64Url(hash(accessToken))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh wow, I didn't know we had to hash and hex the access token to send it.

request: body
})
return checkDecision(data, config)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, why this isn't defined in engine.ts?

@wcalderipe wcalderipe merged commit 20f6105 into main May 7, 2024
5 checks passed
@wcalderipe wcalderipe changed the title base sdk Initial implementation of the Armory SDK May 7, 2024
@wcalderipe wcalderipe deleted the armorySdk/basicTransaction branch May 27, 2024 09:17
mattschoch pushed a commit that referenced this pull request Jun 19, 2024
* base sdk

* config getter

* updated lock

* working example with sdk

* added makefile to sdk

* removed unused package

* re-installed packages to cleanup lock

* remove unused test directory

* add one true test

* added unit tests on utils, narrowed necessary config for evaluate/signRequest functions

* sdk with example for signMessage/Transaction/TypedData working

* Update packages/armory-sdk/src/lib/exceptions.ts

Co-authored-by: William Calderipe <wcalderipe@gmail.com>

* Update packages/armory-sdk/src/lib/exceptions.ts

Co-authored-by: William Calderipe <wcalderipe@gmail.com>

* Remove console.log

* Rename example project to armory-sdk-nodejs

---------

Co-authored-by: William Calderipe <wcalderipe@gmail.com>
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