-
Notifications
You must be signed in to change notification settings - Fork 0
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
seal_reentrant_count
#5
Conversation
d246842
to
c43d921
Compare
seal_reentrant_count
We can do that but better as a separate function to not affect the size of the contract. |
didn't get it |
c43d921
to
a8214fd
Compare
@xgreenx, please take a look |
frame/contracts/src/wasm/mod.rs
Outdated
self.reentrant_count.push(12u8); | ||
12 | ||
} | ||
fn account_entrance_count(&mut self, _account_id: AccountIdOf<Self::T>) -> u32 { unimplemented!() } |
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 it is unimplemented?
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.
don't see a good reason to test this
// to the calling instance. A value of 0 means no reentrancy. | ||
[__unstable__] seal_reentrant_count(ctx) -> u32 => { | ||
ctx.charge_gas(RuntimeCosts::ReentrantCount)?; | ||
Ok(ctx.ext.reentrant_count() as u32) |
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.
You can use ctx.ext.account_entrance_count
here and only add charging for reading of the 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.
ctx.ext.reentrant_count
uses ctx.ext.account_entrance_count
frame/contracts/src/exec.rs
Outdated
} | ||
|
||
fn account_entrance_count(&mut self, account_id: AccountIdOf<Self::T>) -> u32 { | ||
self.frames().filter_map(|f| Some(f.delegate_caller.is_none() && &f.account_id == &account_id)).count() as u32 |
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 guess you can do that in a cheaper way without filtering.
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 have no idea why filtering is considered not cheap enough
params: vec![], | ||
return_type: Some(ValueType::I32), | ||
}], | ||
call_body: Some(body::repeated(r * API_BENCHMARK_BATCH_SIZE, &[ |
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 right will be to calculate how much we need gas for one iteration over frames. And in the functions charge exactly for the count of frames. But we can leave it for official review from the contract-pallet
team if you wish=)
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 idea actually. I propose to move forward with it as it is for now.
seal_reentrant_count
,seal_account_entrance_count
paritytech#11018