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

Added Optional XMTP Parameter #268

Closed
wants to merge 8 commits into from
Closed

Added Optional XMTP Parameter #268

wants to merge 8 commits into from

Conversation

daria-github
Copy link
Contributor

@daria-github daria-github commented Apr 3, 2024

What changed? Why?
Added an optional xmtpMinSupportedVersion parameter to render the XMTP meta tag when it exists. This allows developers creating frames with XMTP support to use this method without additional workarounds.

Also bumps to the latest XMTP validator package version.

How has it been tested?
Updated the unit tests to validate with/without the tag, as well as in local development.

@daria-github daria-github changed the title Added Optional XMTP Content Parameter Added Optional XMTP Parameter Apr 3, 2024
@Zizzamia
Copy link
Contributor

Ciao @daria-github, thank you for the PR.
Left a few notes.

@@ -3,6 +3,7 @@ import { FrameMetadataType } from './types';
type FrameMetadataHTMLResponse = FrameMetadataType & {
ogDescription?: string;
ogTitle?: string;
xmtpMinSupportedVersion?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

// Set the xmtp metadata if the corresponding content exists.
const xmtpHtml = xmtpMinSupportedVersion
? ` <meta property="of:accepts:xmtp" content="${xmtpMinSupportedVersion}" />\n`
: '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just vNext?

Copy link
Contributor

Choose a reason for hiding this comment

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

VNext would be fine too. Unlike Farcaster we actually have versions of the protocol from the start (their spec has versions but in practice everything is VNext).

That means as Frame servers roll out new features like transactions XMTP clients can tell that they need to upgrade to support the Frame.

@@ -44,10 +45,35 @@ describe('getFrameHtmlResponse', () => {
<meta property="fc:frame:post_url" content="https://example.com/api/frame" />
<meta property="fc:frame:refresh_period" content="10" />
<meta property="fc:frame:state" content="%7B%22counter%22%3A1%7D" />
<meta property="of:accepts:xmtp" content="2024-04-03" />
Copy link
Contributor

Choose a reason for hiding this comment

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

What can I read the docs for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/open-frames/standard explains the version negotiation.

Works the same as Farcaster's versioning, except we can't assume all clients are current with VNext.

Copy link
Contributor

Choose a reason for hiding this comment

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

xmtpMinSupportedVersion as API seems odd. Like I why a client is saying MinSupported, and not just "hey this is where I am xmtpVersion, make it work".

Also, to me a vesion should be optional and should just work for free by setting a particular default always.

Or, if it is not set, xmtp should still work anyway and assume the latest stable version. I don't know, trying to think how to make it easier to onboard. In this way just feels an extra step.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like, why should I have of:version and of:accepts:xmtp version. Why just not one. Than what happens if there is a third, fourth, fifth protocal version I need to set. Than things will get out of hand very fast.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like I why a client is saying MinSupported, and not just "hey this is where I am xmtpVersion, make it work".

You can always set it to whatever is the latest version or to VNext if you don't want to think about backwards compatibility with older clients. But if your Frame is taking advantage of new features and you want to make sure that clients don't render half-broken Frames, you can specify the version required of the client. Then clients can see that version and say "Oh, I don't support that" and not render a Frame that's likely to be broken.

Also, to me a vesion should be optional and should just work for free by setting a particular default always.

We do need something in that field, since it is used to tell if a Frame support XMTP payloads at all. Having it default to VNext would be fine. That's ultimately the choice of the library rendering the Frame (onchainkit).

Like, why should I have of:version and of:accepts:xmtp version

The of:version is explicitly the version of the Open Frames spec that's being supported. It doesn't take into account any of the particulars of the Open Frames implementation. If one day XMTP decided to update the signature scheme for it's payloads that wouldn't change anything about the Frame HTML, but some clients would work and some wouldn't. So, two versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds like it might make sense to keep the optional param for min version for the reasons Nick mentioned, but also include a default here so we still always have this meta tag with vnext regardless of whether a user has passed this param. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so of:accepts:$protocol_identifier, let's take a couple of step back. $protocol_identifier can be whatever, and needs a version. Ok, that makes sense.

Can a Frame accept more than one protocol?

Copy link
Contributor

Choose a reason for hiding this comment

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

but also include a default here so we still always have this meta tag with vnext regardless of whether a user has passed this param. wdyt?

We do need some level of developer control over whether they want to support XMTP or not. We can default the version to VNext, but only if the dev has somehow said that they want XMTP support. Otherwise clients are going to think the Frame works and get an error on the first POST.

Can a Frame accept more than one protocol?

Yes, absolutely. You can make a Frame that accepts XMTP and Lens POSTs, and you'd just include two of:accepts tags

@Zizzamia
Copy link
Contributor

Also, v0.11.2 got a nice upgrade for the latest validator package https://github.com/coinbase/onchainkit/releases/tag/v0.11.2

@Zizzamia
Copy link
Contributor

@daria-github did you rebase with main branch?

@daria-github
Copy link
Contributor Author

@daria-github did you rebase with main branch?

@Zizzamia just did, yes!

@Zizzamia
Copy link
Contributor

@daria-github, I took your ideas and give them a nice refresh here #285

@daria-github
Copy link
Contributor Author

Thanks for including this in #285, @Zizzamia! I'll close this PR.

@Zizzamia
Copy link
Contributor

Added both of you in the contributor mention for the latest release https://github.com/coinbase/onchainkit/releases/tag/v0.12.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants