-
Notifications
You must be signed in to change notification settings - Fork 682
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: global contract usage #12886
base: master
Are you sure you want to change the base?
feat: global contract usage #12886
Conversation
…ear/nearcore into stefan/global_contract_account_support
…bal_contract_usage
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12886 +/- ##
==========================================
+ Coverage 70.53% 70.65% +0.12%
==========================================
Files 851 851
Lines 174921 175143 +222
Branches 174921 175143 +222
==========================================
+ Hits 123374 123752 +378
+ Misses 46420 46248 -172
- Partials 5127 5143 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This reverts commit 067c5bb.
ac40cd9
to
b4c4c2f
Compare
ReceiptEnum::GlobalContractDistribution(_) | ||
| ReceiptEnum::Data(_) | ||
| ReceiptEnum::PromiseResume(_) => 0, | ||
ReceiptEnum::GlobalContractDistribution(contract_data) => { |
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.
Costs are charged as part of Action::DeployGlobalContract
processing, why do we consider ReceiptEnum::GlobalContractDistribution
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.
the logic was that the action generates a single outgoing distribution receipt (which later gets cloned) that I can use for the check_balance
calculation
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 understand why your approach works, the question is if this can be implemented by checking for successfully executed Action::DeployGlobalContract
actions
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.
it seems like accessing action execution result isn't straightforward, so we can keep using your approach for now
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 think the alternative is adding a new field in BalanceStats
for gas burnt by global actions
integration-tests/src/test_loop/tests/contract_distribution_cross_shard.rs
Outdated
Show resolved
Hide resolved
total_receipts_cost(config, receipts) | ||
}; | ||
let receipts_cost = | ||
|receipts: &[Receipt], is_outgoing: bool| -> Result<Balance, IntegerOverflowError> { |
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 is very convoluted and invasive way of doing that, I suggest moving it outside of total_receipts_cost
to a dedicated method, something like fn global_contract_deploy_costs(config: &RuntimeConfig, outgoing_receipts: &[Receipt])
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.
added a field in BalanceStats
for gas burnt by global actions, let me know if the approach is better
The PR adds testing and support for the
UseGlobalContract
action. Part of #12716