Add initial Telegram Stamper package #399
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
zkharit
force-pushed
the
zane/telegram-stamper
branch
from
3c9c572 to
ce1b7e4
Compare
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
| @@ -0,0 +1,17 @@ | |||
| import JSDOMEnvironment from "jest-environment-jsdom"; | |||
There was a problem hiding this comment.
I would put this under __tests__ just to make its purpose extra clear :)
There was a problem hiding this comment.
Originally I had it there, but jest thought it was a test file and yelled at me for it not having any tests inside of it.
zkharit
force-pushed
the
zane/telegram-stamper
branch
from
822cb69 to
240782a
Compare
packages/telegram-stamper/README.md
Outdated
| // create a new telegram stamper | ||
| const stamper = new TelegramStamper({ | ||
| apiPublicKey: "...", | ||
| apiPrivateKey: "...", | ||
| }); | ||
|
|
||
| // initialize the telegram stamper, this stores the api key credentials in Telegram Cloud Storage for the first time | ||
| try { | ||
| stamper.init(); | ||
| } catch (err) { | ||
| throw new Error(`Failed initializing Telegram Stamper: ${err}`) | ||
| } |
There was a problem hiding this comment.
I'd be good to simplify this interface to:
const stamper = new TelegramStamper({}); // defaults to creating API key + storing it in cloudstorageAnd if you need to provide an already existing API key:
const stamper = new TelegramStamper({
apiPublicKey: "...",
apiPrivateKey: "...",
});There was a problem hiding this comment.
Thats not actually whats happening here with an empty constructor. The empty constructor is for the case where they have stored a non-expiring api key in cloud storage, and want to use that. The stamper will get the api keys that are currently stored there
There was a problem hiding this comment.
Having said that, maybe its not super likely thats how the tgram stamper will be used? Maybe this should just be removed overall? (from the readme)
There was a problem hiding this comment.
yep yep, I get that. But it could create an API key if nothing's in cloudStorage right? From a dev's POV the construction becomes the same regardless of whether a user has a key or not.
Currently devs will be forced to branch:
if (hasApiKeyInCloudStorage) {
const stamper = new TelegramStamper({});
} else {
const stamper = new TelegramStamper({
apiPublicKey: "...", // also need to worry about how to generate it
apiPrivateKey: "...",
}
}Whereas if we had this behavior (generate key when storage is empty), the init simplifies to:
const stamper = new TelegramStamper({});I guess this is only important if creating and persisting a new key is a common pattern. If the stamper is almost always initiated with an existing key from somewhere else, not worth worrying about.
There was a problem hiding this comment.
To be clear, the tgram stamper does not create any api keys for users, they need to be passed to the stamper
There was a problem hiding this comment.
OOp I wrote that comment before I reloaded and saw your response, but yeah Im not exactly sure it makes sense for the stamper to actually create the api keys, because those api keys will have to be added to the users account somehow meaning that the tgram stamper already needs a valid api key, or something needs to pass the tgram stamper a valid key
There was a problem hiding this comment.
Cool! I'm okay with that. Let's not bundle API key creation in there and always expect public/private key. As you build out the TG mini-app I think patterns will naturally emerge anyway. I suspect you'll need to create a stamper from a bundle (coming from oauth or email auth). That might be a convenience function we want to provide (TelegramStamper.fromBundle(...) would decrypt the bundle and store it in cloudstorage in one go)
There was a problem hiding this comment.
Yeah, I think thatll be a common pattern for sure, that sounds like a nice convenience for sure!
There was a problem hiding this comment.
by the way this brings up a tangential point: it's probably best not to merge this PR until the TG mini-app is built: that way you can get a "feel" for the interface you're building with this stamper, hammer out the bug, etc. The iteration loop I'd recommend is:
- TG stamper PR opened (what you have here)
- TG mini app repo or branch somewhere which imports the codesandbox version of this package
- Try your mini app in TG, look for bugs; look at the code of the mini-app, look for awkwardness. Go back to step 1 and edit to solve for bugs and awkwardness!
and when we're convinced this stamper is a fit for the mini-app: merge PR, publish to NPM, and import the final package in the mini app.
|
|
||
| declare global { | ||
| interface Window { | ||
| Telegram: any; |
There was a problem hiding this comment.
can we be more specific than "any" here? Let's bring this package: https://www.npmjs.com/package/telegram-webapps?
It has all the types we need for CloudStorage:
https://github.com/DavisDmitry/telegram-webapps/blob/d935b6aec22d9361982ba54667eb56faf76b474a/src/index.d.ts#L970
There was a problem hiding this comment.
So this isnt an "official" Telegram package, and I've been going back and forth about including it. Telegram unfortunately only officially offers <script src=telegram-web-app.js />. Its really behind, it terms of how modern apps are developed, and other developers have created much better abstractions like the one you linked. However now were depending on random devs to keep it up to date rather than using the official Telegram source. Given that do you have a strong opinion here?
There was a problem hiding this comment.
what I linked wouldn't be included in the final build I don't think, it's only type definitions (typescript might export them if people import our ts code, but the point remains: it won't be new functionality; just types to guide our code / tests)
There was a problem hiding this comment.
I see, that makes sense. Ill look into that
|
Don't forget to add this new package to https://github.com/tkhq/sdk/blob/main/.codesandbox/ci.json, this will let you build an importable version of it right from this branch so you can iterate and get feedback quickly for integration into a TG mini-app. |
|
didnt know about that ^ awesome!! |
packages/telegram-cloud-storage-stamper/src/util/telegram-environment.ts
Outdated
Show resolved
Hide resolved
packages/telegram-cloud-storage-stamper/src/__tests__/stamp-test.ts
Outdated
Show resolved
Hide resolved
|
|
||
| [](https://www.npmjs.com/package/@turnkey/telegram-cloud-storage-stamper) | ||
|
|
||
| This package contains functions to store a Turnkey API public/private key within [Telegram Cloud Storage](https://core.telegram.org/bots/webapps#cloudstorage). This package also handles stamping a Turnkey request with that API key. It is meant to be used with [`@turnkey/sdk-browser`](https://www.npmjs.com/package/@turnkey/sdk-browser). |
There was a problem hiding this comment.
If we're going to mention @turnkey/sdk-browser specifically here, it would be optimal to have some integration guide type docs to flesh that out.
Technically, if this implements the general stamper interface, it could be used with the lowest level @turnkey/http as well — we could mention that but generally recommend usage with @turnkey/sdk-browser. This also might make sense to... add to @turnkey/sdk-react as well?
In any case, a docs update to https://docs.turnkey.com/api-overview/stamps#stampers at the very least seems relevant
There was a problem hiding this comment.
On the flip side, I can see the argument that we shouldn't because telegram is a pretty separate path and thus shouldn't be included with the other more vanilla browser use cases
There was a problem hiding this comment.
I specifically removed a reference to @turnkey/http here after our convo during standup yesterday 😅 to push users toward our higher level libraries. I do agree that extra doc will be helpful here, and I'm planning to add some :)
| #### Argument Usage | ||
|
|
||
| The `.create()` and `.setSigningKey()` functions take one of the following 4 sets of arguments: | ||
| - No arguments: Use an API key at the default location within Telegram Cloud Storage `TURNKEY_API_KEY` and set that as the signing key |
There was a problem hiding this comment.
What does the default location mean here?
There was a problem hiding this comment.
Just before this section I tried to explain the idea of how storing is done in tg cloud storage and wrote "The Telegram Cloud Storage Stamper will, by default, store the API key used for signing in Telegram Cloud Storage under the key TURNKEY_API_KEY. A Cloud Storage "key" is the index under which a value is stored in Telegram Cloud Storage." Is there something more you think I should clarify here?
| Use an existing key that has been previously stored in Telegram Cloud Storage at the default API key location key location | ||
|
|
||
| ```ts | ||
| import { TelegramCloudStorageStamper } from "@turnkey/telegram-cloud-storage-stamper"; |
There was a problem hiding this comment.
thoughts on calling this just @turnkey/telegram-stamper? Would that be pigeonholing too much? i.e. are there other types of telegram stampers that we could foresee having some day in the future?
There was a problem hiding this comment.
Had this discussion with @r-n-o earlier. It was originally telegram-stamper. There are talks of including Login with Telegram outside of a telegram mini app context. If we want to create a specialized stamper for that, this one will not work because there will not be access to TG cloud storage in a typical web app context so it is possible there is a different telegram stamper that could be developed. But its possible wed just advise users to utilize one our existing stampers if they are not in a telegram context
| // the key used to index Telegram Cloud Storage | ||
| const telegramCloudStorageKey = "telegramCloudStorageKey"; | ||
|
|
||
| // insert the API key in Telegram Cloud Storage |
There was a problem hiding this comment.
for the email auth type use case I imagine?
There was a problem hiding this comment.
Really a convenience method for developers, email auth is a perfectly good use case example, but really tried to be as flexible as possible for developers so they can have the tools they need
zkharit
force-pushed
the
zane/telegram-stamper
branch
from
385d7d1 to
648436c
Compare
| async getAPIKey(key: string = DEFAULT_TURNKEY_CLOUD_STORAGE_KEY) { | ||
| try { | ||
| const apiKey = await this.getItem(key); | ||
|
|
||
| if (!apiKey) { | ||
| return { apiPublicKey: "", apiPrivateKey: "" }; | ||
| } | ||
|
|
||
| return this.parseAPIKey(apiKey as string); | ||
| } catch { | ||
| return { apiPublicKey: "", apiPrivateKey: "" }; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's be clearer to return null when the API key can't be found (instead of a truthy object with empty string props). Also, let's add type annotations here? (-> CloudStorageAPIKey | null)
There was a problem hiding this comment.
changed this to return null in the error case and added the type annotation
| isApiKey(apiKey: CloudStorageAPIKey) { | ||
| return ( | ||
| typeof apiKey.apiPublicKey === "string" && | ||
| typeof apiKey.apiPrivateKey === "string" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The danger is that an empty string is still a string. Is it okay to consider an object with 2 empty keys an "ApiKey"?
This would make for a good unit test target by the way (assert true/false against a bunch of values)
There was a problem hiding this comment.
The usage of this function comes from the need to determine if a value obtained and parsed from tg cloud storage is of type CloudStorageAPIKey, not necessarily that it is a "valid" api key. I changed the name to be isCloudStorageAPIKey and added a comment to be more clear
| ); | ||
| } | ||
|
|
||
| async getItem(key: string) { |
There was a problem hiding this comment.
could be useful to add type annotations to the return value? Or can Typescript infer it? (-> Promise<string>?)
There was a problem hiding this comment.
added the type annotations, typescript could not infer it previously
zkharit
force-pushed
the
zane/telegram-stamper
branch
from
a9ab5ef to
5be3dfc
Compare
Summary & Motivation
Add compatibility for storing Turnkey api keys in Telegram Cloud Storage
How I Tested These Changes
Did you add a changeset?
If updating one of our packages, you'll likely need to add a changeset to your PR. To do so, run
pnpm changeset.pnpm changesetwill generate a file where you should write a human friendly message about the changes. Note how this (example) includes the package name (should be auto added by the command) along with the type of semver change (major.minor.patch) (which you should set).These changes will be used at release time to determine what packages to publish and how to bump their version. For more context see this comment.