-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[bella-ciao] Remove TypeView
#25129
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
base: bella-ciao
Are you sure you want to change the base?
[bella-ciao] Remove TypeView
#25129
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
cgswords
left 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.
LGTM, but: are we ever going to regret removing this information from the gas?
| ) -> PartialVMResult<()> { | ||
| self.charge_instr_with_size( | ||
| Opcodes::CALL_GENERIC, | ||
| ((ty_args.len() + args.len() + 1) as u64).into(), |
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.
Do we care about this cost change?
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.
Not really -- this is only used by the Move CLI outside of the Sui CLI.
I honestly think we should consider removing this entirely and just using the tiered_gas_schedule as that tracks most closely to the metering we do on the Sui side.
cgswords
left 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.
LGTM, assuming we did not want this info anyway. @dariorussi might have opinions, too.
LGTM too, we can chat more but it seems reasonable? |
Description
Removes
TypeViewand its usages.The gas changes are because of the gas charger changes in
dev_utilsgas schedule. Note that there are no semantic changes to the real gas meter on the Sui side though.Test plan
CI
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.