-
Notifications
You must be signed in to change notification settings - Fork 162
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
[WIP] Add XMTP Support #96
Conversation
package.json
Outdated
"packageManager": "[email protected]" | ||
"packageManager": "[email protected]", | ||
"dependencies": { | ||
"@xmtp/frames-validator": "^0.2.0" |
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.
Where is the npm link for this package, I wasn't able to find it.
I am curios to see how big is it on Kb
@@ -724,6 +725,415 @@ __metadata: | |||
languageName: unknown | |||
linkType: soft | |||
|
|||
"@ethersproject/abi@npm:5.7.0, @ethersproject/abi@npm:^5.7.0": |
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.
These are quite a lot of dependencies is taking in.
@@ -0,0 +1,28 @@ | |||
import { FramePostPayload } from '@xmtp/frames-validator/dist/src/types'; | |||
import { FrameValidationResponse } from '../../core/types'; | |||
import { validateFramesPost } from '@xmtp/frames-validator'; |
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 wonder if this can be implemented independently.
I am a bit worry of how big is @xmtp/frames-validator
dependency for OnchainKit.
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.
Frames validator itself is very small, but it relies on xmtp-js
. The good news is that we have someone already working on removing our dependency on ethers, which is the biggest dependency of that library.
Just for my own understanding, is there a plan to run onchainkit
in browsers? Or is this going to remain purely for servers? If we want browser support, there are a few other changes I'd need to make to remove dependencies on node:crypto
.
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.
For context, any feature in OnchainKit has to meet three bars:
- being able to run on Web client
- being able to run on latest Backend Node.js
- being able to run on React Native
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.
As OnchainKit has to work out of the box for all Coinbase products using these three case scenarios.
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.
OK. That's very good to know. We can work with those requirements. RN is the most challenging, since it doesn't have a built in crypto
module or wasm
support, but we have a RN SDK so none of this is new to us.
I'll get back to you on a new timeline with those requirements in mind.
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.
All the current functionality only makes sense on a server, and a lot of it is specifically designed around Next.js
Yeah that's fair. I think we did a lot of simplification with our Frames approach by using a third-party provider for validating messages.
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.
Part of me is like, we should create a https://github.com/Zizzamia/a-frame-in-100-lines version that showcase how to use your library in backend for node.js and just call it the day.
After all OnchainKit is a client library, and it avoids the backend part as much as possible.
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.
Or in some way, OnchainKit knows how to recognize if is XMTP, an that's it, if that's the case we advice to use your library to complete other parts.
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.
You see what I mean, I think it's fair to find a way to make the two working well together, instead of trying to force OnchainKit to be something is not.
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.
These are good points. I'm going to take a step back and try again with a more abstract approach, where the validation logic and cryptographic libraries don't have to be embedded in onchainkit
. That avoids the dependency issues and library bloat, plus it lets you potentially support more than just XMTP Frames.
Closing this, in favour of #116 |
What changed? Why?
verifiedWalletAddress
field. This is done without any network requests, since XMTP POST payloads can be verified as a self-contained payload.Notes to reviewers
This will need to wait on a PR in
@xmtp/frames-validator
to make it support being imported from CommonJS modules. That should land tomorrow.How has it been tested?