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

fix: investigate transactions missing overrides #4657

Open
ltyu opened this issue Oct 10, 2024 · 1 comment · Fixed by #4658
Open

fix: investigate transactions missing overrides #4657

ltyu opened this issue Oct 10, 2024 · 1 comment · Fixed by #4658
Labels

Comments

@ltyu
Copy link
Contributor

ltyu commented Oct 10, 2024

Problem

In the SDK and other places, we occasionally see this

'getDeployedInterchainAccount(uint32,address,address,address)'
](
originDomain,
config.owner,
originRouterAddress,
destinationIsmAddress,
),

this.router(contracts)['setDestinationGas((uint32,uint256)[])'](
remoteConfigs,
),

Where a transaction is created without an override. This is normally acceptable because it is optional, but in our case, we rely on a transactionOverrides config so overrides is always required. We often end up fetching the overrides using this pattern in most of the code:

const overrides = this.multiProvider.getTransactionOverrides(chain);
await this.runIfOwner(chain, routingHook, async () => {
this.logger.debug(
{
chain,
routingHookAddress: routingHook.address,
routingConfigs,
},
'Setting routing hooks',
);
return this.multiProvider.handleTx(
chain,
routingHook.setHooks(routingConfigs, overrides),
);
});

This is problematic because devs sometimes forget that overrides are required so we end up with unexpected behaviors such as when deploying to Sei and missing maxPriorityFeePerGas:

{"code":-32000,"message":": insufficient fee"}}

Solution

  • Consider adding a transformation within handleTx() or anywhere else that adds the override because handleTx is often called right after.
  • Add a linter to make overrides required
  • Any other solutions where devs don't have to think about adding overrides
  • Also part of this task is to fix any places that are missing overrides
  • Perhaps use multiprovider.getTransaction()
@ltyu ltyu added the sdk label Oct 10, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 10, 2024
### Description
This partially fixes
#4657,
specifically made to get warp apply to work on Sei

### Backward compatibility
Yes

### Testing
Manual - deployed
@ltyu
Copy link
Contributor Author

ltyu commented Oct 21, 2024

Reopening this after incident review discussion. Worth to investigate

context https://www.notion.so/hyperlanexyz/Renzo-Warp-Apply-1206d35200d680498504f53eccb66e75

@ltyu ltyu reopened this Oct 21, 2024
@ltyu ltyu changed the title fix: transactions missing overrides fix: investigate transactions missing overrides Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Sprint
Development

Successfully merging a pull request may close this issue.

1 participant