implement Chain.buildMessageForDest and allow partial extraArgs #102
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 andrevmatos, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Coverage Report |
ccip-sdk/src/evm/index.ts
Outdated
| data: hexlify(message_.data ?? '0x'), | ||
| tokenAmounts: message.tokenAmounts ?? [], | ||
| feeToken: message.feeToken ?? ZeroAddress, | ||
| extraArgs: hexlify((this.constructor as typeof EVMChain).encodeExtraArgs(message.extraArgs)), |
There was a problem hiding this comment.
| extraArgs: hexlify((this.constructor as typeof EVMChain).encodeExtraArgs(message.extraArgs)), | |
| extraArgs: hexlify((this.constructor as typeof EVMChain).encodeExtraArgs(_message.extraArgs)), |
Should be _message instead?
PabloMansanet
left a comment
PabloMansanet
left a comment
There was a problem hiding this comment.
LGTM in terms of behaviour (once the bug found by Amine is addressed) though I have some concerns on readability and naming.
I think this feels like a common builder pattern where the interface accepts both the built product and the builder itself (to then supply defaults) so I would leverage the users familiarity with the concept of builders to make the interfaces easier to read.
There's also some naming in the implementations that could use some work (e.g. message_ vs message is error prone, compared to populatedMessage vs message or, if you go with my original suggestion, message vs messageBuilder.
Overall not a merge-blocker but I'd like your take on the naming options 👍
| describe: "Address of the Solana tokenReceiver (if different than program's receiver)", | ||
| }, | ||
| account: { | ||
| alias: 'receiver-object-id', |
There was a problem hiding this comment.
What's the benefit of aliasing instead of having a separate optional argument? receiverObjectIds seem conceptually pretty different to solana accounts so maybe it's a bit cleaner to split it (no strong opinion though).
There was a problem hiding this comment.
From CLI's perspective, it's not different: each is an array of address, and they're mutually exclusive (either you're sending to Solana and populating accounts, or you're sending to Sui and populating receiverObjectIds in extraArgs; they also serve semantically the same purpose: a list of dependent addresses needed for the receiver callback in dest;
it's inevitable to expose them a little in order to allow them to pass important specific parameters like these, which we (ccip-tools-ts) can't derive/guess for them, but my idea here is to make it as cross-compatible as possible
There was a problem hiding this comment.
Sounds good, happy to keep as-is.
ccip-sdk/src/aptos/index.ts
Outdated
| message, | ||
| }: Parameters<Chain['getFee']>[0]): Promise<bigint> { | ||
| return getFee(this.provider, router, destChainSelector, message) | ||
| const message_ = populateDefaultMessageForDest(message, networkInfo(destChainSelector).family) |
There was a problem hiding this comment.
| const message_ = populateDefaultMessageForDest(message, networkInfo(destChainSelector).family) | |
| const populatedMessage = populateDefaultMessageForDest(message, networkInfo(destChainSelector).family) |
NIT: I think being explicit here helps readability (this applies elsewhere in this PR)
ccip-sdk/src/evm/index.ts
Outdated
| destChainSelector, | ||
| message, | ||
| }: Parameters<Chain['getFee']>[0]): Promise<bigint> { | ||
| const message_ = populateDefaultMessageForDest(message, networkInfo(destChainSelector).family) |
There was a problem hiding this comment.
Mixing message_ and message in this function feels error-prone and a bit confusing. I'd rename message to populatedMessage and be consistent in only using populatedMessage after this point, as it's a superset of message anyway, which saves the reader from having to think whether message is still OK to use or not.
Even better (for me) would be to go with the builder naming (message vs messageBuilder), I'll talk more about that later.
ccip-sdk/src/chain.ts
Outdated
| * @param message - AnyMessage (from source), containing at least `receiver` | ||
| * @returns A message suitable for `sendMessage` to this destination chain family | ||
| */ | ||
| populateDefaultMessageForDest(message: RequestMessage): AnyMessage |
There was a problem hiding this comment.
This reads to me like a builder pattern. WDYT about renaming this to something like:
| populateDefaultMessageForDest(message: RequestMessage): AnyMessage | |
| BuildMessageForDest(builder: MessageBuilder): AnyMessage |
I ask because I find RequestMessage a bit strange in terms of naming. Am I requesting a message? Is it a message request? To whom?
Things like the send API could take a parameter of type message: AnyMessage | MessageBuilder to make it obvious they can supply defaults if needed.
There was a problem hiding this comment.
Yes, agree with the renaming, if it makes things clearer
…Args
this function populates a partial message (`RequestMessage` type is
exported and replaces stricter `AnyMesssage`) with default extraArgs and
massage other parameters; it allows sending a simple token transfer by
just calling: `chain.sendMessage({router, dest, message: { receiver,
tokenAmounts: [{ token, amount}] }})`
andrevmatos
force-pushed
the
feat/token-transfer2
branch
from
dfdebfa to
20eb8cd
Compare
andrevmatos
changed the title
andrevmatos
force-pushed
the
feat/token-transfer2
branch
from
20eb8cd to
5c083fd
Compare
4 participants
RequestMessagetype is exported, and is a relaxed version ofAnyMessage, which requires onlyreceiver, and replaces it in the exported typesgetFee,generateUnsignedSendMessageandsendMessageChain.buildMessageForDest, is able to convert aRequestMessageinto stricterAnyMessage(needed byRouter.ccipSend) by populating default values where they make sense, specially forextraArgsgasLimit,tokenReceiver, etcGenericExtraArgsV2(for EVM, Aptos, TON)