-
Notifications
You must be signed in to change notification settings - Fork 244
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: meta transactions #1097
feat: meta transactions #1097
Conversation
🦋 Changeset detectedLatest commit: 71a2472 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
f2e5094
to
d97d2a6
Compare
packages/transactions/src/schema.ts
Outdated
export function encodeTransaction(transaction: Transaction | SignedTransaction) { | ||
return serialize(SCHEMA, transaction); | ||
} | ||
|
||
/** | ||
* Borsh-decode a Transaction instance from a buffer | ||
* @param bytes Buffer data to be decoded | ||
*/ | ||
export function decodeTransaction(bytes: Buffer) { | ||
return deserialize(SCHEMA, Transaction, bytes); | ||
} |
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.
Initially I wanted to opportunistically refactor the Transaction
and SignedTransaction
classes to decouple the class definitions from SCHEMA
, but ultimately decided it was too much unrelated effort for a half measure.
I opted to leave these new methods in place and use them in favor of the encode/decode class methods but they're not strictly relevant to this PR.
65c0642
to
47509fd
Compare
} | ||
|
||
module.exports = { | ||
sendNearThroughRelayer, |
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 created the cookbook
examples from a new template from an in-review PR that supports testing snippets here. When that PR is landed I can add integration tests for these.
import { Signature } from './signature'; | ||
|
||
interface MessageSigner { | ||
sign(message: Uint8Array): Promise<Uint8Array>; |
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'm not super attached to this, it was just my way of inching away from coupling transaction signing methods and network while avoiding stepping into the current state of keystores and their 1:1 account-network mapping. Definitely open to alternatives or tweaks to this approach.
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.
👍 Let's go with it 💖
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.
Good job on this! Most looks good, just a couple questions around the necessity of some parts :)
"@near-js/cookbook": patch | ||
--- | ||
|
||
Add support for delegate actions and meta transactions |
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.
Add support for delegate actions and meta transactions | |
Add support for meta-transactions/delegate actions |
I believe meta-transactions & delegate actions are synonymous
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.
Interesting, I made the distinction mostly in the context of relayers:
- delegate action is an action signed by the relay caller
- meta transaction is a transaction signed by the relayer
I omitted the hyphen to reflect its title and usage in NEP 366
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 NEP closer I see it doesn't actually refer to any thing as a "meta transaction", it's just a flow facilitated by senders and relayers. Probably clearer to omit meta transactions
altogether but that kinda buries the lede 😅
sorry misclick |
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.
Good job 👏
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.
Nice work -- looking good. Just spotted a few things which I added comments about in-line and was thinking about some naming tweaks.
Just one thing that I noticed while looking through the PR was the DelegateAction type being local to the transactions package -- I would've expected it to be in the @near-js/types
package which has a Transaction type that would need to reference all of the action types for accuracy.
I think to avoid circular references between the types package and the rest of the packages we're going to need to pull almost all of the types into the types package and only keep internal/local types in the individual packages that use the @near-js/types package -- I think this is why a Transaction type exists in the types package, but because Action types don't, the Transaction type in the types package defines actions
as an type. We should have a chat about this and it's not any kind of a blocker for this PR to land for us to iterate on.
DeleteAccount, | ||
} from './actions.js'; | ||
|
||
export type NonDelegateAction = |
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.
Naming feels a bit counter-intuitive -- could this be DelegableAction
instead to make it more explicit and to describe what it is rather than what it isn't? It's less likely to change if for some reason we introduce new actions in the future that have a similar restriction (cannot be delegated) even though right now the only action that can't be delegated is a DelegateAction. Could add a JSDoc on the type to describe how DelegateActions are not delegable if we want to call attention to that specifically
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 it more closely I see it's actually ignoring the union types altogether:
{ actions: Array<AddKey> }
=>actions = [new SignedDelegate({})]
❌{ actions: Array<AddKey | DeleteKey> }
=>actions = [new SignedDelegate({})]
✅
It looks like by having a union with > 1 members, the common shape is demoted to their common Assignable
base class and so anything with constructor(properties: any)
will pass the union. This wasn't something I knew about union types 💡
So with the current implementation being set dressing, I think options are:
- double down on this approach and incorporate delegability into the class hierarchy
- implement the chain-side logic in the client with runtime checks that throw exceptions when a non-delegate action is added to the signed delegate
- ignore for now and let the chain fail them
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.
Let's let the chain fail them and re-visit the classes later for ideal typesafety 👍
import { Signature } from './signature'; | ||
|
||
interface MessageSigner { | ||
sign(message: Uint8Array): Promise<Uint8Array>; |
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.
👍 Let's go with it 💖
Co-authored-by: Daryl Collins <daryl@darylcollins.com>
The crux of the issue is that many of these are classes implementing methods with heavy dependencies not appropriate for
The |
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 think this is shippable at this stage 💖
This PR implements functionality for delegate actions and meta transactions for NEP 366: Meta Transactions.
For
@near-js/transactions
, this adds:DelegateAction
actionsDelegateAction
sDelegateAction
In
@near-js/accounts
I've addedAccount
methods:signedDelegate
builds and signs a delegate actionsignAndSendMetaTransaction
builds and signs a meta transaction derived from the delegate actionA new cookbook example in included to demonstrate the usage of the new
Account
methods.