-
Notifications
You must be signed in to change notification settings - Fork 249
chore: added gas estimation api to planner #11953
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
Conversation
Deploying agoric-sdk with
|
Latest commit: |
6e44475
|
Status: | ✅ Deploy successful! |
Preview URL: | https://14f75664.agoric-sdk.pages.dev |
Branch Preview URL: | https://fraz-gas-estimation.agoric-sdk.pages.dev |
7dd7769
to
e2b88d2
Compare
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 clear on how close to done this is supposed to be.
}; | ||
|
||
export const planTransfer = ( | ||
type GasEstimator = (chainName: AxelarChain) => { |
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.
a GasEstimator is a function that takes a chainName and gives back a thing with methods? maybe call it...
type GasEstimator = (chainName: AxelarChain) => { | |
type MakeGasEstimator = (chainName: AxelarChain) => { |
const evmGasEstimator = gasEstimator(evm); | ||
const [gmpAccountFee, gmpWalletFee, gmpReturnFee] = await Promise.all([ | ||
evmGasEstimator.getFactoryContractEstimate(), | ||
evmGasEstimator.getWalletEstimate(), |
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.
How much padding/slippage is in the estimate? There has to be some just to account for things changing between the time of the estimate and the time of execution.
IIUC, The consequences of estimating too low (customer's stuff doesn't work) are drastically worse than guessing too high (Agoric spends some extra BLD; I think we even get a refund).
So I'd like the padding to be around 3x.
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.
OTOH, I gather the padding is already 20x in some cases. hm.
// TODO: Rather than hard-code, derive from Axelar `estimateGasFee`. | ||
// https://docs.axelar.dev/dev/axelarjs-sdk/axelar-query-api#estimategasfee | ||
fee: make(feeBrand, 15_000_000n), | ||
fee: make(feeBrand, gmpAccountFee), |
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 suggest adding a TODO to skip the account fee if the account already exists (which we can tell from accountIdByChain
in the portfolio node)
'function executeFactory(bytes32 commandId, string sourceChain, string sourceAddress, bytes payload)', | ||
'function executeWallet(bytes32 commandId, string sourceChain, string sourceAddress, bytes payload)', | ||
]; | ||
const GAS_ESTIMATOR_OWNER = 'agoric1owner'; |
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.
why is it ok to not use a real bech32 address? add a comment?
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.
this is what was provided in the consructor argument owner
for the GasEstimator contract so thats what we're using here. technically it can be any arbitrary value
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'll add a comment to the code as well
'0x' + bytes.map(n => n.toString(16).padStart(2, '0')).join(''); | ||
|
||
const gasEstimatorContracts = { | ||
mainnet: {}, |
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 take it there's an implicit TODO here.
is this PR supposed to be DRAFT?
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.
should this PR only be merged when we have all contracts set up on mainnet and testnet? or do we take the approach of merging this with some contracts deployed, and add more as we go?
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.
Any PR that is forward progress without regressions is fine to merge, provided we're clear about the scope and we keep track somewhere of any remaining essential work.
I'd like to understand better what would happen if we ran this code in production. If we would hit critical problems, please add a TODO. A (draft) issue might help too.
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 progress
evmRpcMap[ | ||
`${chainConfig.chainInfo.namespace}:${chainConfig.chainInfo.reference}` | ||
]; | ||
|
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.
We're recomputing all these things to get access to the relevant EVM chain provider. Instead you can pass evmCtx.evmProviders
when calling makeGasEstimatorKit
in plan-deposit.ts
params: any[] = [], | ||
fromAddress?: string, | ||
): Promise<bigint> => { | ||
const provider = new ethers.JsonRpcProvider(rpcUrl); |
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.
This should be obtained from evmCtx.evmProviders
.
const axelarConf = | ||
clusterName === 'mainnet' ? axelarConfig : axelarConfigTestnet; | ||
const chainConfig = axelarConf[chainName]; | ||
|
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 should happen in main.ts
. That's where we setup all the configs based on the network before the planner.
sourceChain, | ||
destinationChain, | ||
gasLimit: gasLimit.toString(), | ||
sourceTokenSymbol: gasToken, |
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.
Would it fail if the gasToken
is not defined? Perhaps gasToken
should not be optional?
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.
its not required in all cases where this function is used. specifically the eth -> agoric tx
|
||
const data = await response.json(); | ||
return data; | ||
}; |
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 data
should be typed. Based on docs for estimateGasFee, the response seems to have this shape:
{
"totalFee": "BigNumber",
"isExpressSupported": "boolean",
"baseFee": "BigNumber",
"expressFee": "BigNumber",
"executionFee": "BigNumber",
"executionFeeWithMultiplier": "BigNumber",
"gasLimit": "BigNumber",
"gasLimitWithL1Fee": "BigNumber",
"gasMultiplier": "float",
"minGasPrice": "BigNumber"
}
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.
thats in case we pass the showDetailedFees
to the api, otherwise its just a number
|
||
const generateAaveSupplyPayload = ( | ||
contractAddress: string, | ||
payment: string, |
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.
shouldn't payment
be bigint
? I think in the contract code amounts are usually in bigint
const gasEstimator = makeGasEstimatorKit({ | ||
alchemyApiKey: evmCtx.alchemyApiKey, | ||
clusterName: evmCtx.clusterName, | ||
}); |
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 should be setup in main.ts
and then passed to startEngine
via it's powers object.
@frazarshad I’m trying to understand the flow:
What I’m unclear on is: where exactly is the |
mainnet: {}, | ||
testnet: { | ||
Avalanche: '0x0010F196F5CD0314f68FF665b8c8eD93531112Fe', | ||
}, |
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.
Would be helpful to include the snowtrace link for this contract
const GAS_ESTIMATOR_CONTRACT_ABI = [ | ||
'function executeFactory(bytes32 commandId, string sourceChain, string sourceAddress, bytes payload)', | ||
'function executeWallet(bytes32 commandId, string sourceChain, string sourceAddress, bytes payload)', | ||
]; |
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.
@frazarshad where is the source code for this contract? Add a comment to share the source code link?
|
||
// arbitrary value. it just mimicks a valid command id for axelar | ||
const COMMAND_ID = | ||
'0xddea323337dfb73c82df7393d76b2f38835d5c2bda0123c47d1a2fc06527d19f'; |
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.
Include the tx
link which has this command id.
Also this is a testnet transaction I think? Perhaps we would create a map to command id(s) for different chains and networks?
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.
this is an arbitrary value it is not linked to any contract and not specific for any chain.
in actuality this is a randomly generated key created by axelar assigned to each individual transaction
since we cant generate our own, we use a value that looks like it
readPublished: query.readPublished, | ||
spectrum, | ||
cosmosRest, | ||
}, |
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 stuff in evmCtx
should be spread out in the powers object:
{
readPublished: query.readPublished,
spectrum,
cosmosRest,
...evmCtx
}
@rabi-siddique yes that is accurate it is being put into the params arg. these args are passed to the |
amount: NatAmount, | ||
feeBrand: Brand<'nat'>, | ||
makeGasEstimator: MakeGasEstimator, | ||
accountIdByChain: Record<string, `${string}:${string}:${string}`>, |
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.
accountIdByChain: Record<string, `${string}:${string}:${string}`>, | |
accountIdByChain: Record<string, AccountId>, |
and in fact the record key is not just any string. Unless there's something I'm not seeing, I suggest:
accountIdByChain: Record<string, `${string}:${string}:${string}`>, | |
accountIdByChain: StatusFor['portfolio']['accountIdByChain'], |
fee: make(feeBrand, gmpWalletFee), // KLUDGE. | ||
fee: make(feeBrand, gmpWalletFee * padding), // KLUDGE. |
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 it still a KLUDGE?
246196e
to
e1a4a98
Compare
export const gasLimitEstimates: GasLimitEstimates = { | ||
WALLET: 1_209_435n, | ||
AAVE_SUPPLY: 276_809n, | ||
}; |
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.
Just confirmed with @frazarshad
WALLET
has the gasLimit for makeAccount flow.
AAVE_SUPPLY
is the limit used for these operations: supply / withdraw / deposit for burn
Maybe change AAVE_SUPPLY
name to something generic
sourceChain: AxelarChain | 'agoric', | ||
destinationChain: AxelarChain | 'agoric', | ||
gasLimit: bigint, | ||
gasToken?: string, |
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.
Why not including the destinationContractAddress
or field similar to that
|
||
const getFactoryContractEstimate = async () => { | ||
const gasLimit = gasLimitEstimates['AAVE_SUPPLY']; | ||
|
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.
This tx will be executed by the smart wallet so maybe it should be renamed to geSmartWalletEstimate
instead getFactoryContractEstimate
?
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.
that would make me think of "SmartWallet" in the sense of the Agoric walletFactory. But maybe it would help other readers.
services/ymax-planner/src/main.ts
Outdated
cosmosRest, | ||
signingSmartWalletKit, | ||
now: Date.now, | ||
axelarApiAddress, |
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.
maybe axelarApiAddress
should be part of evmCtx
.
if (errors.length) { | ||
throw AggregateError(errors, 'Could not get balances'); | ||
} | ||
const makeGasEstimator = prepareGasEstimator(powers); |
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 we should have one makeGasEstimator
instance maybe instantiated in main.ts
instead of creating a new one everytime handleDeposit
is called
makeGasEstimator: MakeGasEstimator, | ||
accountIdByChain: Record<string, `${string}:${string}:${string}`>, |
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.
These last two parameters are not like the others; I think they should be bundled into a record.
export const planTransfer = async (
dest: PoolKey,
amount: NatAmount,
feeBrand: Brand<'nat'>,
{ gasEstimator, accountIdByChain }: PlanTransferPowers,
): MovementDesc[] => {
const data = Number(await response.json()); | ||
return data; |
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.
const data = Number(await response.json()); | |
return data; | |
const body = await response.json(); | |
return BigInt(body); |
Also: https://docs.axelarscan.io/gmp#estimateGasFee shows "BigNumber" in its output, but doesn't explain what that is. If it's not guaranteed to be an integer for us, then this will fail. Should we be prepared for that by e.g. rounding up?
const data = Number(await response.json()); | |
return data; | |
const body = await response.json(); | |
try { | |
return BigInt(body); | |
} catch (err) { | |
log('⚠️ Axelar gas API non-integer response, trying to round up', body); | |
const parts = `${body}`.trim().match(/^([0-9]+)([.]0*|[.]([0-9]+))?$/); | |
if (!parts) throw err; | |
const intPart = BigInt(parts[1]); | |
return parts[3] ? intPart + 1n : intPart; | |
} |
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 its safe as is. as BigNumber is most likely this type https://docs.ethers.org/v5/api/utils/bignumber/
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.
That page also doesn't document a restriction to integers, but the documentation about migrating from ethers v5 to v6 does:
Keep in mind, just like BigNumber, a ES2020 BigInt can only operate on integers.
So I'm convinced that a non-integer value would be sufficiently surprising to merit propagating the resulting exception without special handling.
services/ymax-planner/src/support.ts
Outdated
export const gasLimitEstimates: GasLimitEstimates = { | ||
WALLET: 1_209_435n, | ||
AAVE_SUPPLY: 276_809n, | ||
}; |
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.
How were these values determined?
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.
avg of values observed when these txs were run on mainnet as well as testnet. e.g here you can observe the Gas Limit & Used by Txn
section. This value represents the "Used by Txn" part of this section.
They varied across txs but not my a large enough margin to indicate they will have a significant effect on the gasEstimate
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.
My point is that we should have an explanation here (or link to one), in case things do change in the future.
services/ymax-planner/test/mocks.ts
Outdated
// Mock fetch | ||
globalThis.fetch = async (url: string, options?: any) => { | ||
if (url.includes('estimateGasFee')) { | ||
return { | ||
ok: true, | ||
json: async () => JSON.parse(options.body).gasLimit, | ||
} as Response; | ||
} | ||
throw new Error(`Unmocked fetch call to: ${url}`); | ||
}; |
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.
This is a huge smell. Library code should not be accessing an ambient fetch
, but rather requiring it as an inbound power passed down from startEngine
.
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.
Also, passing all of fetch
around when you only need read-only access to a certain part of web space is excess authority. Consider using makeWebRd.
e1a4a98
to
66a3a6e
Compare
case 'withdraw': | ||
details = { | ||
fee: AmountMath.make( | ||
graph.feeBrand, | ||
gasEstimator | ||
? (await gasEstimator.getWalletEstimate( | ||
chainOf(edge.dest) as AxelarChain, | ||
)) * padding | ||
: 30_000_000n, | ||
), | ||
}; | ||
break; | ||
case 'supply': | ||
details = { | ||
fee: AmountMath.make( | ||
graph.feeBrand, | ||
gasEstimator | ||
? (await gasEstimator.getWalletEstimate( | ||
chainOf(edge.dest) as AxelarChain, | ||
)) * padding | ||
: 30_000_000n, | ||
), | ||
}; | ||
break; |
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.
As noted, these cases appear to be identical:
case 'withdraw': | |
details = { | |
fee: AmountMath.make( | |
graph.feeBrand, | |
gasEstimator | |
? (await gasEstimator.getWalletEstimate( | |
chainOf(edge.dest) as AxelarChain, | |
)) * padding | |
: 30_000_000n, | |
), | |
}; | |
break; | |
case 'supply': | |
details = { | |
fee: AmountMath.make( | |
graph.feeBrand, | |
gasEstimator | |
? (await gasEstimator.getWalletEstimate( | |
chainOf(edge.dest) as AxelarChain, | |
)) * padding | |
: 30_000_000n, | |
), | |
}; | |
break; | |
case 'withinEvmViaAxelar': { | |
const estimate = await gasEstimator.getWalletEstimate( | |
chainOf(edge.dest) as AxelarChain, | |
); | |
details = { | |
fee: AmountMath.make(graph.feeBrand, padFeeEstimate(estimate)), | |
}; | |
break; |
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.
supply and withdraw are currently have the same estimate but will be different in the future, collapsing the switch cases but keeping them as distinct feeModes
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.
You said in another reply that fees for same-chain transactions are calculated based on a message from Agoric to that chain, so why would the calculations differ for into vs. out of a pool on the other chain?
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 total fee is for both fee_for_agoric_to_evm + execution_fee_on_evm
the first part is the same but the second part varies because supply and withdraw executions will run different code on evm
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 total fee is for both fee_for_agoric_to_evm + execution_fee_on_evm the first part is the same but the second part varies because supply and withdraw executions will run different code on evm
That reads to me like different inputs to the same calculation rather than different calculations—perhaps the gasEstimator method should take src and dest inputs? Please add a TODO to revisit this (even if the final determination is that it should not change).
case 'cctpReturn': | ||
details = { | ||
fee: AmountMath.make( | ||
graph.feeBrand, | ||
gasEstimator | ||
? (await gasEstimator.getWalletEstimate( | ||
chainOf(edge.src) as AxelarChain, | ||
)) * padding | ||
: 30_000_000n, | ||
), | ||
}; | ||
break; |
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.
Shouldn't this case use getReturnFeeEstimate
?
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.
getReturnFeeEstimate
is (a bit confusingly) the detail.evmGas
on the makeAccountEvm
feeMode. It represents the gas amount required for the return trip to get the new account address back to agoric
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 must be missing something, because I don't see any calls to getReturnFeeEstimate
at all.
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.
yes we're not using it at the moment, since its estimate is unreliable at the moment. we have instead hardcoded to 200_000_000_000_000n
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 what it would cost to move the hard-coded value to a config file / environment...
for example, getReturnFeeEstimate
could look it up there.
const getWalletEstimate = async (chainName: AxelarChain) => | ||
queryAxelarGasAPI( | ||
AGORIC_CHAIN, | ||
chainName, | ||
gasLimitEstimates.Wallet, | ||
BLD_TOKEN, | ||
); |
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.
Given that getWalletEstimate
is used by rebalanceMinCostFlowSteps
for transactions that don't leave a host EVM chain, why is this call involving AGORIC_CHAIN at all?
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.
this is basically the estimate for the tx that will send the message to the remote evm account.
Once the message has been received it will execute a tx within the evm ecosystem.
so while the tx is bounded to the chain. the message (for which we're making the estimate) comes from agoric
* details which vary by the traversed link. | ||
* - toUSDN: transferring into USDN transfer reduces the *payload* (e.g., $10k | ||
* might get reduced to $9995) | ||
* - makeEvmAccount: the fee for executing the factory contract to create a new |
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.
Where is "factory account" defined/described? There should be a link here.
/** | ||
* Gas estimation interface - mirrors services/ymax-planner/src/gas-estimation.ts | ||
* - getWalletEstimate: Estimate gas fees for remote wallet operations on the specified chain | ||
* - getFactoryContractEstimate: Estimate gas fees for executing the factory contract on the specified chain |
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.
Here too, provide a reference for "factory contract".
case 'cctpReturn': | ||
details = { | ||
fee: AmountMath.make( | ||
graph.feeBrand, | ||
gasEstimator | ||
? (await gasEstimator.getWalletEstimate( | ||
chainOf(edge.src) as AxelarChain, | ||
)) * padding | ||
: 30_000_000n, | ||
), | ||
}; | ||
break; |
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 must be missing something, because I don't see any calls to getReturnFeeEstimate
at all.
fee: { | ||
brand: Object @Alleged: fee brand (BLD) {}, | ||
value: 30000000n, | ||
value: 3628305n, |
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.
Seems like gas value is reduced?
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.
this is a mock value. its not an accurate estimate of what we will see in testnet and mainnet
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 gas estimate logic looks good for now. The hardcoded values should help avoid manual intervention during testing. LGTM.
After addressing Richard's comments we should merge this PR. I suggest we create issues for the enhancements we need to do later on.
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.
Thanks for the responsiveness. Final suggestions attached.
* Link to Factory and Wallet contracts: | ||
* https://github.com/agoric-labs/agoric-to-axelar-local/blob/cd6087fa44de3b019b2cdac6962bb49b6a2bc1ca/packages/axelar-local-dev-cosmos/src/__tests__/contracts/Factory.sol |
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.
This is decent, but I see no description here or there of what the Factory and Wallet contracts are/what they do/etc. Please take an action item to provide documentation of that.
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.
agoric-labs/agoric-to-axelar-local#30 made a ticket for later
case 'withdraw': | ||
details = { | ||
fee: AmountMath.make( | ||
graph.feeBrand, | ||
gasEstimator | ||
? (await gasEstimator.getWalletEstimate( | ||
chainOf(edge.dest) as AxelarChain, | ||
)) * padding | ||
: 30_000_000n, | ||
), | ||
}; | ||
break; | ||
case 'supply': | ||
details = { | ||
fee: AmountMath.make( | ||
graph.feeBrand, | ||
gasEstimator | ||
? (await gasEstimator.getWalletEstimate( | ||
chainOf(edge.dest) as AxelarChain, | ||
)) * padding | ||
: 30_000_000n, | ||
), | ||
}; | ||
break; |
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 total fee is for both fee_for_agoric_to_evm + execution_fee_on_evm the first part is the same but the second part varies because supply and withdraw executions will run different code on evm
That reads to me like different inputs to the same calculation rather than different calculations—perhaps the gasEstimator method should take src and dest inputs? Please add a TODO to revisit this (even if the final determination is that it should not change).
79423f8
to
6e44475
Compare
Ref #11953 ## Description * Simplify validation of `axelarApiAddress` * Improve `gasEstimator` error output * Define type GasEstimator in solver code (where it is needed) * Make better use of existing utility functions * Clean up import ordering
Description
Includes code to add gas estimation in planner. It estimates two different types of gas values. Outgoing as well as incoming.
For the outgoing txs i.e Agoric -> EVM. We have a record of
gasLimitEstimates
that are representative of real world gas amounts used during makeAccount / supply / withdraw tx.At the moment there are only two types of gas limits we've added. one for the execution of the factory contract. the other for the execution of the wallet contract.
In the future in place of the
Wallet
gas limit, we would have a list of gas Limits representing each item in the matrix {supply, withdraw} x {Aave, Compund, Beefy} since each is silghtly different and it would be more efficient to have the minimum possible value for eachFor incoming, the API call the estimation seems to be off according to our testing. The API gives an esitmate which is 20x higher than the value we use in testnet (0.0002 vs 0.005)
Testing Considerations
Tested in this Tx: https://testnet.axelarscan.io/gmp/0xa1b399894a97b70bf38302d1297fa1f6b2fd198b1fffc0529c0bb1e5c89725af-4
It supplied the build amount for the outgoing tx but also calculated the eth amount for the incoming tx