-
Notifications
You must be signed in to change notification settings - Fork 18
feat: Solana relayFeeCalculator and gasPriceOracle #980
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
base: epic/svm-client
Are you sure you want to change the base?
Conversation
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
I can confirm that the gas price oracle is giving reasonable estimates for priority fees/base fees (assuming that the fill transaction only requires a single signature). The relay fee calculator simulation is still failing somewhere in the fill instruction, and I haven't figured out if this is an issue with how the instruction is being created or if I am just testing with relay data values that would cause the SvmSpoke to revert, but even if it is incorrect, this should at most require a small change in |
src/arch/svm/SpokeUtils.ts
Outdated
* @param extraSeed An optional extra seed. Defaults to 0. | ||
* @returns The PDA for the State account. | ||
*/ | ||
export async function getStatePda(programId: string, extraSeed = 0): Promise<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.
I generally just copied the format for these functions from #978
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.
#978 has been merged now so it should be OK to rebase to include those (fingers crossed for no merge conflicts...)
const recentPriorityFees = await provider.getRecentPrioritizationFees(instructionAddresses).send(); | ||
|
||
const nonzeroPrioritizationFees = recentPriorityFees.map((value) => value.prioritizationFee).filter((fee) => fee > 0); | ||
const totalPrioritizationFees = nonzeroPrioritizationFees.reduce((acc, fee) => acc + fee, BigInt(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.
Am definitely open for ideas on how to optimize the priority fee calculation. I can get nonzero priority fees for the most part, presumably since TOKEN_PROGRAM_ADDRESS
is one of the instructionAddresses
, but a lot of slots suggest a priority fee of zero for one reason for another, and this RPC by default will query 100 slots, so it's possible under this method that we obtain a prioritization fee which is dominated by data from older slots.
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 JSON-RPC docs for Solana say they retain fee data for 150 slots - do you only get the last 100 on query?
In any case, would we consider just taking the last n slots - i.e. 25? That'd give 10 seconds of coverage, which is pretty long in relative terms. Is there additionally some minimum priority fee that we might want to set, just so that we don't submit at 0?
Signed-off-by: bennett <[email protected]>
I'm now getting both reasonable gas price estimations and reasonable compute unit estimations. |
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
src/arch/svm/SpokeUtils.ts
Outdated
* @param extraSeed An optional extra seed. Defaults to 0. | ||
* @returns The PDA for the State account. | ||
*/ | ||
export async function getStatePda(programId: string, extraSeed = 0): Promise<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.
#978 has been merged now so it should be OK to rebase to include those (fingers crossed for no merge conflicts...)
): Promise<Address<string>> { | ||
const [associatedToken] = await getProgramDerivedAddress({ | ||
programAddress: ASSOCIATED_TOKEN_PROGRAM_ADDRESS, | ||
seeds: [owner.toBuffer(), SvmAddress.from(tokenProgramId).toBuffer(), mint.toBuffer()], |
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.
Is the user of Buffer
here a hangover from the contracts implementation? Seeds in Kit have to conform to type Seed = ReadonlyUint8Array | string;
, so it should be OK to pass strings in; I think this should work:
seeds: [owner.toBuffer(), SvmAddress.from(tokenProgramId).toBuffer(), mint.toBuffer()], | |
seeds: [owner.toAddress(), tokenProgramId, mint.toAddress()], |
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 actually not too sure how the string encoding is supposed to work here, but each character is treated as a byte, so passing in a string here will have getProgramDerivedAddress
interpret the input as 66 byte strings, not 32 bytes.
mint: mint.toV2Address(), | ||
delegate: statePda, | ||
owner: relayer.toV2Address(), | ||
amount: amount.toBigInt(), |
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.
Neat - TIL!
const { unsignedTx: _unsignedTx } = opts; | ||
|
||
// Cast the opaque unsignedTx type to a solana-kit CompilableTransactionMessage. | ||
const unsignedTx = _unsignedTx as CompilableTransactionMessage; |
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.
It'd be nice to verify this via superstruct - then we'd inherit type inference as well.
const recentPriorityFees = await provider.getRecentPrioritizationFees(instructionAddresses).send(); | ||
|
||
const nonzeroPrioritizationFees = recentPriorityFees.map((value) => value.prioritizationFee).filter((fee) => fee > 0); | ||
const totalPrioritizationFees = nonzeroPrioritizationFees.reduce((acc, fee) => acc + fee, BigInt(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.
The JSON-RPC docs for Solana say they retain fee data for 150 slots - do you only get the last 100 on query?
In any case, would we consider just taking the last n slots - i.e. 25? That'd give 10 seconds of coverage, which is pretty long in relative terms. Is there additionally some minimum priority fee that we might want to set, just so that we don't submit at 0?
const isEthersProvider = provider instanceof providers.Provider; | ||
// Exit here if we need to estimate on Solana. | ||
if (!isEthersProvider) { |
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 we should make chainId
a mandatory field in GasPriceEstimateOptions
, because we can't resolve the Solana "chain Id" via an RPC call, so this won't scale beyond one non-evm/svm chain.
...opts, | ||
chainId: CHAIN_IDs.SOLANA, | ||
}; | ||
return solana.messageFee(provider as SolanaProvider, optsWithDefaults); |
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.
Is type inference not working on provider
? It might work if line 60 is changed if (!provider instanceof providers.Provider)
. We might alternatively want a superstruct discriminator to handle it for us.
@@ -213,7 +213,9 @@ export class QueryBase implements QueryInterface { | |||
? Promise.resolve({ maxFeePerGas: _gasPrice }) | |||
: getGasPriceEstimate(provider, { chainId, baseFeeMultiplier, priorityFeeMultiplier, transport, unsignedTx }), | |||
] as const; | |||
const [nativeGasCost, { maxFeePerGas: gasPrice }] = await Promise.all(queries); | |||
const [nativeGasCost, _gasPriceEstimate] = await Promise.all(queries); | |||
// It should be safe to cast to an EvmGasPriceEstimate here since QueryBase is only used for EVM chains. |
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.
It's probably a good use case for an assert(EvmGasPriceEstimate.is(gasPriceEstimate))
. We'd need superstruct defs for this.
src/utils/SpokeUtils.ts
Outdated
function _getRelayDataHashSvm(relayData: RelayData, destinationChainId: number): string { | ||
const bufferFromHexString = (hex: string, littleEndian: boolean = false) => { | ||
const buffer = Buffer.from(hex.slice(2), "hex"); | ||
if (buffer.length < 32) { | ||
const zeroPad = Buffer.alloc(32); | ||
buffer.copy(zeroPad, 32 - buffer.length); | ||
return littleEndian ? zeroPad.reverse() : zeroPad; | ||
} | ||
return littleEndian ? buffer.slice(0, 32).reverse() : buffer.slice(0, 32); | ||
}; | ||
const bufferFromInt = (num: BigNumber, byteLength: number, littleEndian: boolean = true) => { | ||
const buffer = Buffer.from(num.toHexString().slice(2), "hex"); | ||
if (buffer.length < byteLength) { | ||
const zeroPad = Buffer.alloc(byteLength); | ||
buffer.copy(zeroPad, byteLength - buffer.length); | ||
return littleEndian ? zeroPad.reverse() : zeroPad; | ||
} | ||
return littleEndian ? buffer.slice(0, byteLength).reverse() : buffer.slice(0, byteLength); | ||
}; | ||
const contentToHash = Buffer.concat([ | ||
bufferFromHexString(relayData.depositor), | ||
bufferFromHexString(relayData.recipient), | ||
bufferFromHexString(relayData.exclusiveRelayer), | ||
bufferFromHexString(relayData.inputToken), | ||
bufferFromHexString(relayData.outputToken), | ||
bufferFromInt(relayData.inputAmount, 8), | ||
bufferFromInt(relayData.outputAmount, 8), | ||
bufferFromInt(toBN(relayData.originChainId), 8), | ||
bufferFromInt(relayData.depositId, 32, false), | ||
bufferFromInt(toBN(relayData.fillDeadline), 4), | ||
bufferFromInt(toBN(relayData.exclusivityDeadline), 4), | ||
bufferFromHexString(getMessageHash(relayData.message)), | ||
bufferFromInt(toBN(destinationChainId), 8), | ||
]); | ||
return keccak256(contentToHash); | ||
} |
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.
Damn; this one caught me by surprise. Can we stick it in src/arch/svm/SpokeUtils.ts
? We might also want to relocate the EVM part as well, so getRelayDataHash()
should be pretty thin as a result.
Co-authored-by: Paul <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
This should implement the relayFeeCalculator (and by extension, the gasPriceOracle) for svm. It will contain some other utilities which are used by those structures.