-
Notifications
You must be signed in to change notification settings - Fork 45
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: create token bridge #34
Conversation
import { TokenBridgeContracts } from './types/TokenBridgeContracts'; | ||
|
||
export type CreateTokenBridgeFetchTokenBridgeContractsParams = { | ||
inbox: 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.
Should we use the inbox address for this, or the rollup 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.
It's fine if we use rollup
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 leaning towards not changing this now. In getTokenBridgeContracts
we get the inbox from the OrbitTokenBridgeCreated event, and then we need to get the bridge address, and finally the rollup address.
Then, here, in createTokenBridgeFetchTokenBridgeContracts, we would need to get the inbox address from the rollup anyway.
This back and forth is what doesn't convince me for now. If we ever see the need to use rollup
here in the future (in other use cases), it might make sense to refactor it then.
import { TokenBridgeContracts } from './types/TokenBridgeContracts'; | ||
|
||
export type CreateTokenBridgeFetchTokenBridgeContractsParams = { | ||
inbox: 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.
It's fine if we use rollup
// checking retryables execution | ||
const orbitChainRetryableReceipts = await txReceipt.waitForRetryables({ | ||
orbitPublicClient: nitroTestnodeL2Client, | ||
}); | ||
expect(orbitChainRetryableReceipts).toHaveLength(2); | ||
expect(orbitChainRetryableReceipts[0].status).toEqual('success'); | ||
expect(orbitChainRetryableReceipts[1].status).toEqual('success'); |
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.
Added checks for successful retryables execution
src/utils/gasOverrides.ts
Outdated
export function applyGasOverrides({ | ||
gasOverrides, | ||
estimatedGas, | ||
defaultGas, | ||
}: { | ||
gasOverrides: GasOverrideOptions; | ||
estimatedGas?: bigint; | ||
defaultGas: bigint; | ||
}) { | ||
const baseEstimatedGas = gasOverrides.base ?? estimatedGas ?? defaultGas; | ||
|
||
return gasOverrides.percentIncrease | ||
? baseEstimatedGas + (baseEstimatedGas * gasOverrides.percentIncrease) / 100n | ||
: baseEstimatedGas; | ||
} |
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 can simplify this a bit and just have a pure function for percent increase:
export function applyGasOverrides({ | |
gasOverrides, | |
estimatedGas, | |
defaultGas, | |
}: { | |
gasOverrides: GasOverrideOptions; | |
estimatedGas?: bigint; | |
defaultGas: bigint; | |
}) { | |
const baseEstimatedGas = gasOverrides.base ?? estimatedGas ?? defaultGas; | |
return gasOverrides.percentIncrease | |
? baseEstimatedGas + (baseEstimatedGas * gasOverrides.percentIncrease) / 100n | |
: baseEstimatedGas; | |
} | |
export function applyPercentIncrease({ | |
base, | |
percentIncrease = 0 | |
}: { | |
base: bigint; | |
percentIncrease?: bigint | |
}) { | |
return base + (base * percentIncrease) / 100n | |
} |
// potential gas overrides (gas limit) | ||
if (gasOverrides && gasOverrides.gasLimit) { | ||
request.gas = applyGasOverrides({ | ||
gasOverrides: gasOverrides.gasLimit, | ||
estimatedGas: request.gas, | ||
defaultGas: createTokenBridgeDefaultGasLimit, | ||
}); | ||
} |
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 can make this a bit simpler (combined with the change from the other comment):
// potential gas overrides (gas limit) | |
if (gasOverrides && gasOverrides.gasLimit) { | |
request.gas = applyGasOverrides({ | |
gasOverrides: gasOverrides.gasLimit, | |
estimatedGas: request.gas, | |
defaultGas: createTokenBridgeDefaultGasLimit, | |
}); | |
} | |
// potential gas overrides (gas limit) | |
if (gasOverrides && gasOverrides.gasLimit) { | |
request.gas = applyPercentIncrease({ | |
base: gasOverrides.gasLimit.base ?? request.gas, | |
percentIncrease: gasOverrides.gasLimit.percentIncrease, | |
}); | |
} |
Don't think we really need to have defaults in here, it's good enough to expose as constants so the consumer can do:
createTokenBridge({
...,
gasOverrides: {
gasLimit: {
base: createTokenBridgeDefaultGasLimit
}
}
})
todos:
getTokenBridgeContracts
function onCreateTokenBridgeTransactionReceipt
that parses logs and returns contractsgetTokenBridgeContracts
to existing token bridge integration tests