-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Create benchmark #14916
Create benchmark #14916
Conversation
⏱️ 1h 59m total CI duration on this PR
🚨 2 jobs on the last run were significantly faster/slower than expected
|
43f2715
to
d468098
Compare
d468098
to
43f2715
Compare
6c002e4
to
44e71be
Compare
43f2715
to
e036793
Compare
44e71be
to
9161080
Compare
e036793
to
e4239a8
Compare
9161080
to
b728e81
Compare
e4239a8
to
d92055d
Compare
ef3c832
to
e2fcd91
Compare
d92055d
to
881aa71
Compare
e2fcd91
to
4caabaf
Compare
881aa71
to
e569dd2
Compare
4caabaf
to
e5d6e5d
Compare
e569dd2
to
7723f05
Compare
e5d6e5d
to
bf6221c
Compare
7723f05
to
54bc94c
Compare
bf6221c
to
115f8f6
Compare
54bc94c
to
5d15334
Compare
115f8f6
to
124c67e
Compare
d287494
to
939ead6
Compare
b4f354a
to
2f74ae7
Compare
939ead6
to
f6d98cc
Compare
2f74ae7
to
b082a3c
Compare
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.
Great, thanks for adding! You need to regenerate package/module bytes though, to get this included in the benchmark?
- Regenerate
framework_usacases
package:// Run "cargo run -p module-publish" to generate the file `raw_module_data.rs`. - Add a test to check it works (manually is ok):
fn test_native_vm_benchmark_transaction() {
@igor-aptos should know more about this workflow
@@ -78,6 +78,8 @@ pub enum TransactionTypeArg { | |||
SmartTablePicture1MWith1KChangeExceedsLimit, | |||
DeserializeU256, | |||
SimpleScript, | |||
PermissionedTransfer, | |||
APTTransfer, |
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.
Few things:
- Use
Apt
like in other enum variants - How is this related to
AptFaTransfer
? Isn't it the same? I am fine having both, I think it would be nice to unify transfer under one use cases: coin, fa, permissioned, possibly veiled in the future
@@ -232,6 +232,8 @@ pub enum EntryPoints { | |||
/// there to slow down deserialization & verification, effectively making it more expensive to | |||
/// load it into code cache. | |||
SimpleScript, | |||
APTPermissionedTransfer, |
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.
nit: can we add these variants next to other transfers?
5135b7b
to
d74365f
Compare
9b05820
to
1230cf4
Compare
8514dc2
to
5d7f12a
Compare
primary_fungible_store::grant_apt_permission(source, &permissioned_signer, amount); | ||
aptos_account::transfer(&permissioned_signer, to, amount); | ||
|
||
permissioned_signer::destroy_permissioned_handle(handle); |
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 permissioned_signer
retains its APT transfer permissions after the transfer completes. To maintain proper permission hygiene, call primary_fungible_store::revoke_apt_permission(source, &permissioned_signer)
before destroying the handle. This ensures no permissions persist beyond their intended use.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
5d7f12a
to
de618cd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
de618cd
to
aa7cdfe
Compare
aa7cdfe
to
145eaa1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
Description
Added new transaction types for APT token transfers, including a permissioned transfer option that uses the permissioned signer framework. This enables both standard APT transfers and permissioned transfers through the transaction generator and e2e benchmark framework.
How Has This Been Tested?
The functionality is integrated into the existing e2e benchmark framework and transaction generator test infrastructure. The new transfer types are added to the test suite alongside other transaction types.
Key Areas to Review
permissioned_transfer.move
implementing both standard and permissioned transfer functionsType of Change
Which Components or Systems Does This Change Impact?
Checklist