-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
tx context #20000
base: main
Are you sure you want to change the base?
tx context #20000
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
20000!!! |
and it's mine!!! |
@@ -257,7 +257,7 @@ mod checked { | |||
metrics, | |||
move_vm, | |||
&mut temporary_store, | |||
tx_context, | |||
tx_context.clone(), |
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.
to change this API to have a TXContext
and not a &mut
requires a change in the executor API (so all version) and I am wondering whether we should do it or just let it go.
Thoughts?
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.
Why do we need an API change here?
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.
this API used to take a &mut TxContext
(boundary between core and execution) though here we take the TxContext
by value. So we clone the mutable reference in order to pass it by value, which is kind of lame as we could have passed the context by value in the first place
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.
though there may be issues the way the code is changed now, looking into it and we'll talk more
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 added a fix for what I think was an oversight on my part. Take a look at the 2 commit
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.
well it turns out the "fix" I put breaks a genesis test exactly because it's updating the context in what seems an incompatible way.
Now I am really confused
gas_charger, | ||
advance_epoch_pt, | ||
); | ||
tx_ctx.add_ids_created(temporary_store.objects_created_count() as u64); |
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.
we could do this operation according to the result but I figured this is the simplest way to do it and it should be safe, thought?
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.
Seems fine to me
db9b0f0
to
4b575ae
Compare
Description
Describe the changes or additions included in this PR.
Test plan
How did you test the new or updated feature?
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.