-
Notifications
You must be signed in to change notification settings - Fork 50
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
Change verifyPayersBalanceForTransactionExecution #331
Change verifyPayersBalanceForTransactionExecution #331
Conversation
contracts/FlowFees.cdc
Outdated
let tokenVault = payerAcct.borrow<&FlowToken.Vault>(from: /storage/flowTokenVault) | ||
?? panic("Unable to borrow reference to the default token vault") | ||
|
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.
Shouldn't this be deleted since you do it below?
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.
good catch! Forgot to delete it after changing it.
f6dcf6c
to
fb2d0f0
Compare
Any Idea why this would fail the js tests? |
@sisyphusSmiling @alilloig Can y'all look at the CI here and try to help figure out why the JS tests are failing? |
@joshuahannan at a quick glance, I believe it's related to the updates to Flow CLI which make versions beyond 0.42.0 incompatible with flow-js-testing. I was able to fix this earlier today in this PR to flow-ft, and will take a closer look in the morning |
3733: Upgrade core contracts r=janezpodhostnik a=janezpodhostnik ... to include: onflow/flow-core-contracts#331 Co-authored-by: Janez Podhostnik <[email protected]>
minimumRequiredBalance
should actually be equal toreservedBalance
.The
maxTransactionFee
was removed fromminimumRequiredBalance
as it is generally much smaller then thereservedBalance
and only covers some edge cases, so its simpler to just not include it (as pointed out by @bluesign here).