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(evm): update bytecode for v2.1.0 #1324

Merged
merged 2 commits into from
Feb 20, 2022
Merged

Conversation

fish-sammy
Copy link
Contributor

@fish-sammy fish-sammy commented Feb 18, 2022

Description

Todos

  • Unit tests
  • Manual tests
  • Documentation
  • Connect epics/issues
  • Tag type of change
  • Upgrade handler

Steps to Test

Expected Behaviour

Other Notes

@fish-sammy fish-sammy added enhancement New feature or request next release Required for the next release labels Feb 18, 2022
@@ -72,15 +72,15 @@ const (
}
]`
AxelarGatewayCommandMintToken = "mintToken"
mintTokenMaxGasCost = 200000
mintTokenMaxGasCost = 150000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

AxelarGatewayCommandBurnToken = "burnToken"
burnTokenMaxGasCost = 200000
burnTokenMaxGasCost = 400000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is burn so much more expensive now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

external token

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, can we be smarter about it then? Have two constants, one for external erc20 burn and one for our wrapped ones. And depending on the command, the appropriate one is added. We're filling up our batches fairly often already, and by doubling the burn cost for our wrapped tokens too, we'll be much less efficient. Avalanche, where we have most throughput rn, has had pretty high variable gas costs recently ($10-50 per batch).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but I'd rather save that for later and fix it separately. External tokens also affect other commands like deployToken and mintToken.

@@ -72,15 +72,15 @@ const (
}
]`
AxelarGatewayCommandMintToken = "mintToken"
mintTokenMaxGasCost = 200000
mintTokenMaxGasCost = 150000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mints seem to only cost 80000 rn. I would imagine it should be cheaper with the optimizations? Should we keep it more around 120k?

And I guess in the worst case, we can still retry a batch with a higher gas limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"rn" is irrelevant. these numbers are from the latest version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was that it seemed like we optimized gas costs quite a bit, so if "rn" (mainnet version) it's 80000, I'd imagine it'll be even lower in the latest version, so just surprised by that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimizations were applied to the new version which carries a lot of new features. It's never expected to be better than the previous version but better than a non-optimized latest version.

@fish-sammy fish-sammy merged commit 963a0d3 into main Feb 20, 2022
@fish-sammy fish-sammy deleted the feat/update-bytecode-v2.1.0 branch February 20, 2022 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next release Required for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants