-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: use fallback predicate estimation within transaction builder #1428
Conversation
}; | ||
|
||
Ok( | ||
(latest_chain_executor_version > LATEST_STATE_TRANSITION_VERSION) |
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.
@xgreenx do we also need to check the consensus values version in case the gas prices 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.
Do we want to hold off the merge until this is answered?
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.
No, we don't need to check consensus parameters for estimation. We may want to check remote version with local version to be sure that we use the latest consensus parameters. If local consensus parameters outdated, then we want to download them one more time.
We can do that because without new state transition function fuel-core
can't support new gas costs variant -> If the state transition function is the same, but consensus parameters are different, we know how to download them since it is supported version.
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 may want to check remote version with local version to be sure that we use the latest consensus parameters. If local consensus parameters outdated, then we want to download them one more time.
I believe we already do this. I need to double-check, though. cc @hal3e
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.
Currently we cache the consensus_parameters
once when we connect to the node. I do not see any update mechanism. We will have to add this.
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.
Yeah, we're not updating it, we just check the supported version during the build.
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.
Update: We'll implement this update mechanism soon as part of a follow-up work, so we don't block the release here.
I have moved the
provider
fallback into theTransaction
trait via theDryRunner
.I had to move
DryRunner
andDryRun
intofuels_core::types
because of the wasm target.BREAKING CHNGE:
DryRunner
andDryRun
tofuels_core::types
Checklist