Skip to content
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

Merged
merged 57 commits into from
Feb 12, 2024
Merged

feat: create token bridge #34

merged 57 commits into from
Feb 12, 2024

Conversation

spsjvc
Copy link
Member

@spsjvc spsjvc commented Dec 28, 2023

todos:

  • don't hardcode gas limit for parent chain tx
  • add padding to retryable fees
  • custom gas token support
    • create util for approving the retryable fees to the token bridge creator
    • add integration test for custom gas token
  • create getTokenBridgeContracts function on CreateTokenBridgeTransactionReceipt that parses logs and returns contracts
    • try using the registry
    • add assertions related to getTokenBridgeContracts to existing token bridge integration tests
  • clean up abi
  • add example for creating a token bridge

@spsjvc spsjvc changed the title wip token bridge creator needs cleanup feat: create token bridge Jan 9, 2024
import { TokenBridgeContracts } from './types/TokenBridgeContracts';

export type CreateTokenBridgeFetchTokenBridgeContractsParams = {
inbox: Address;
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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.

yarn.lock Show resolved Hide resolved
@TucksonDev TucksonDev marked this pull request as ready for review February 1, 2024 20:56
src/createTokenBridgePrepareTransactionReceipt.ts Outdated Show resolved Hide resolved
src/createTokenBridgePrepareTransactionRequest.ts Outdated Show resolved Hide resolved
src/createTokenBridge-ethers.ts Outdated Show resolved Hide resolved
src/createTokenBridge-ethers.ts Outdated Show resolved Hide resolved
src/types/TokenBridgeContracts.ts Outdated Show resolved Hide resolved
src/createTokenBridge.integration.test.ts Outdated Show resolved Hide resolved
src/createTokenBridge.integration.test.ts Show resolved Hide resolved
import { TokenBridgeContracts } from './types/TokenBridgeContracts';

export type CreateTokenBridgeFetchTokenBridgeContractsParams = {
inbox: Address;
Copy link
Member Author

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

yarn.lock Show resolved Hide resolved
Comment on lines +120 to +126
// 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');
Copy link
Contributor

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

Comment on lines 14 to 28
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;
}
Copy link
Member Author

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:

Suggested change
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
}

Comment on lines 67 to 74
// potential gas overrides (gas limit)
if (gasOverrides && gasOverrides.gasLimit) {
request.gas = applyGasOverrides({
gasOverrides: gasOverrides.gasLimit,
estimatedGas: request.gas,
defaultGas: createTokenBridgeDefaultGasLimit,
});
}
Copy link
Member Author

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):

Suggested change
// 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
    }
  }
})

@spsjvc spsjvc requested a review from TucksonDev February 12, 2024 17:08
@spsjvc spsjvc merged commit acad187 into main Feb 12, 2024
5 checks passed
@spsjvc spsjvc deleted the feat-token-bridge-creator branch February 12, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants