-
Notifications
You must be signed in to change notification settings - Fork 214
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
feat: added initial version of Frame Transaction types #211
Conversation
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.
LGTM, one comment but I think we can address in future if we see different namespaces (e.g., solana
) being used
src/frame/types.ts
Outdated
* Note: exported as public Type | ||
*/ | ||
export type FrameTransactionResponse = { | ||
chainId: `eip155:${number}`; // A CAIP-2 chain ID to identify the tx network |
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.
In principle I think the namespace could be broader than just eip155
, but this is probably fine for now
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.
Yeah you right, how would look like a type that supports Solana transactions as well?
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.
The spec is pretty broad and TS doesn't presently accommodate making a regex-validated string type.
Couple options:
// weakest
type ChainId = string;
// stronger, still weak
type ChainId = `${string}:${string}`;
// strongest, requires manual upkeep
type Namespace = 'eip155' | 'solana';
type Reference = string;
type ChainId = `${Namespace}:${Reference}`;
What might be best is to use a simpler type and just make sure we pass through some validation at runtime
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 developed a package one year back here for chain agnostic which incorporates solana as well. We could use this lol
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.
+1 making the change now
src/frame/types.ts
Outdated
abi: Abi; // The contract ABI for the contract to call. | ||
data?: Hex; // The data to send with the transaction. | ||
to: Hex; // The address of the contract to call. | ||
value: string; // The amount of Ether to send with the transaction. |
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.
Possibly bigint
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 would say both are okay here as long as we are very specific on formatting. BigInt is more implicit to be WEI.
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.
Looking at the example, it does appear to be WEI :
https://www.notion.so/warpcast/Frame-Transactions-Public-Draft-v2-9d9f9f4f527249519a41bd8d16165f73
value — value of ether to send with the transaction in wei
params: {
abi: [...], // JSON ABI of the function selector and any errors
to: "0x00000000fcCe7f938e7aE6D3c335bD6a1a7c593D",
data: ""
value: "984316556204476",
},
So we should probably accept BigInt on the interface and ensure it is serialzed as a string in the frame render.
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.
+1 making the change now
src/frame/types.ts
Outdated
export type FrameTransactionEthSendParams = { | ||
abi: Abi; // The contract ABI for the contract to call. | ||
data?: Hex; // The data to send with the transaction. | ||
to: Hex; // The address of the contract to call. |
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.
Can this not be an Address
type? Though it's arguably the same behind the scenes.
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.
+1 Address seems to be more common with applications that use Viem https://viem.sh/docs/glossary/types.html#address
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.
+1 making the change now
What changed? Why?
Cooking some initial types for Frame transactions.
We are following the latest spec https://warpcast.notion.site/Frame-Transactions-Public-Draft-v2-9d9f9f4f527249519a41bd8d16165f73
We aim to enhance those types further as we gain insights from the latest spec version.