aptos-core icon indicating copy to clipboard operation
aptos-core copied to clipboard

Published 20 hours ago • verified publisher icon aptos-labs

[framework] implement immovable objects

Open davidiw opened this issue 1 year ago • 5 comments
trafficstars

Make it so that objects can be made immovable via the constructor ref. Even if other entities have access to the constructor ref.

Add the functionality to fungible asset to make immovable stores by default for assets that opt into the behavior.

Also offer a root_owner function to determine the deepest node that owns a nested object.

See https://github.com/aptos-foundation/AIPs/pull/414

Description

Type of Change

  • [x] New feature
  • [ ] Bug fix
  • [ ] Breaking change
  • [ ] Performance improvement
  • [ ] Refactoring
  • [ ] Dependency update
  • [ ] Documentation update
  • [ ] Tests

Which Components or Systems Does This Change Impact?

  • [ ] Validator Node
  • [ ] Full Node (API, Indexer, etc.)
  • [ ] Move/Aptos Virtual Machine
  • [x] Aptos Framework
  • [ ] Aptos CLI/SDK
  • [ ] Developer Infrastructure
  • [ ] Other (specify)

How Has This Been Tested?

Lots of unit tests

Key Areas to Review

There's really not that much code...

Checklist

  • [x] I have read and followed the CONTRIBUTING doc
  • [x] I have performed a self-review of my own code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I identified and added all stakeholders and component owners affected by this change as reviewers
  • [x] I tested both happy and unhappy path of the functionality
  • [x] I have made corresponding changes to the documentation

davidiw avatar May 02 '24 06:05 davidiw

⏱️ 2h 12m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-targeted-unit-tests 45m 🟩 🟩 
rust-move-unit-coverage 39m 🟩 🟩 
rust-move-tests 20m 🟩 🟩 
rust-lints 11m 🟩 🟩 
run-tests-main-branch 8m 🟩 🟩 
general-lints 4m 🟩 🟩 
check-dynamic-deps 2m 🟩 🟩 
semgrep/ci 47s 🟩 🟩 
file_change_determinator 22s 🟩 🟩 
file_change_determinator 20s 🟩 🟩 
permission-check 10s 🟩 🟩 
permission-check 7s 🟩 🟩 
permission-check 6s 🟩 🟩 
permission-check 5s 🟩 🟩 

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
rust-targeted-unit-tests 23m 15m +55%

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

trunk-io[bot] avatar May 02 '24 06:05 trunk-io[bot]

There's object transfer and fa transfer, nomenclature is hard :grimacing:

davidiw avatar May 02 '24 07:05 davidiw

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 57.6%. Comparing base (1af48e9) to head (6766805). Report is 25 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff            @@
##             main   #13175    +/-   ##
========================================
  Coverage    57.5%    57.6%            
========================================
  Files         833      834     +1     
  Lines      198121   198399   +278     
========================================
+ Hits       113999   114334   +335     
+ Misses      84122    84065    -57

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 02 '24 07:05 codecov[bot]

Currently, if you first call to enable ungated transfer, can to make it immovable doesn't do anything. Should we move the check to one of transfer implementation functions?

"Move" is also overloaded a lot. Can we just be more explicit? CannotChangeOwner, or ImmutableOwner, or something like that?

igor-aptos avatar May 02 '24 14:05 igor-aptos

Currently, if you first call to enable ungated transfer, can to make it immovable doesn't do anything. Should we move the check to one of transfer implementation functions?

Did you type this on a phone :P

I might be missing your point, this locks all possible transfers of the object after it has been set immovable. I'm not sure there's a logical error, we can always surface different errors and have more checks if we want to be more pedantic.

I'll update the AIP and think about the name... Frozen, Immovable, Untransferable sound okay ImmutableOwner is more explicit but sounds ... lame :thinking:

davidiw avatar May 02 '24 14:05 davidiw

Forge is running suite realistic_env_max_load on 6766805bde50fa31553af6217acc2ce6358933d7

github-actions[bot] avatar May 10 '24 00:05 github-actions[bot]

Forge is running suite compat on 01b24e7e3548382dd25440b39a0438a993387f12 ==> 6766805bde50fa31553af6217acc2ce6358933d7

github-actions[bot] avatar May 10 '24 00:05 github-actions[bot]

:white_check_mark: Forge suite compat success on 01b24e7e3548382dd25440b39a0438a993387f12 ==> 6766805bde50fa31553af6217acc2ce6358933d7

Compatibility test results for 01b24e7e3548382dd25440b39a0438a993387f12 ==> 6766805bde50fa31553af6217acc2ce6358933d7 (PR)
1. Check liveness of validators at old version: 01b24e7e3548382dd25440b39a0438a993387f12
compatibility::simple-validator-upgrade::liveness-check : committed: 5721 txn/s, latency: 5792 ms, (p50: 5100 ms, p90: 8900 ms, p99: 10000 ms), latency samples: 223120
2. Upgrading first Validator to new version: 6766805bde50fa31553af6217acc2ce6358933d7
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1853 txn/s, latency: 15747 ms, (p50: 19300 ms, p90: 22600 ms, p99: 23100 ms), latency samples: 90800
3. Upgrading rest of first batch to new version: 6766805bde50fa31553af6217acc2ce6358933d7
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1301 txn/s, latency: 21112 ms, (p50: 24900 ms, p90: 28800 ms, p99: 29300 ms), latency samples: 74200
4. upgrading second batch to new version: 6766805bde50fa31553af6217acc2ce6358933d7
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3539 txn/s, latency: 8915 ms, (p50: 9600 ms, p90: 12600 ms, p99: 12900 ms), latency samples: 145100
5. check swarm health
Compatibility test for 01b24e7e3548382dd25440b39a0438a993387f12 ==> 6766805bde50fa31553af6217acc2ce6358933d7 passed
Test Ok

github-actions[bot] avatar May 10 '24 00:05 github-actions[bot]

:x: Forge suite realistic_env_max_load failure on 6766805bde50fa31553af6217acc2ce6358933d7

two traffics test: inner traffic : committed: 7699 txn/s, latency: 5038 ms, (p50: 4800 ms, p90: 5800 ms, p99: 10500 ms), latency samples: 3364800
two traffics test : committed: 100 txn/s, latency: 1975 ms, (p50: 1900 ms, p90: 2200 ms, p99: 7100 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.206, avg: 0.202", "QsPosToProposal: max: 0.247, avg: 0.235", "ConsensusProposalToOrdered: max: 0.464, avg: 0.418", "ConsensusOrderedToCommit: max: 0.375, avg: 0.356", "ConsensusProposalToCommit: max: 0.789, avg: 0.773"]
Max round gap was 1 [limit 4] at version 1015074. Max no progress secs was 4.58435 [limit 15] at version 3420848.
Test Ok
Trailing Log Lines:
stresschaos.chaos-mesh.org "group-3-cpu-stress" deleted
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/report.rs:41"},"thread_name":"main","hostname":"forge-e2e-pr-13175-1715300094-6766805bde50fa31553af6217acc2ce63","timestamp":"2024-05-10T00:29:13.032915Z","message":"Test Ok"}
test CompositeNetworkTest ... ok
Test Statistics: 
two traffics test: inner traffic : committed: 7699 txn/s, latency: 5038 ms, (p50: 4800 ms, p90: 5800 ms, p99: 10500 ms), latency samples: 3364800
two traffics test : committed: 100 txn/s, latency: 1975 ms, (p50: 1900 ms, p90: 2200 ms, p99: 7100 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.206, avg: 0.202", "QsPosToProposal: max: 0.247, avg: 0.235", "ConsensusProposalToOrdered: max: 0.464, avg: 0.418", "ConsensusOrderedToCommit: max: 0.375, avg: 0.356", "ConsensusProposalToCommit: max: 0.789, avg: 0.773"]
Max round gap was 1 [limit 4] at version 1015074. Max no progress secs was 4.58435 [limit 15] at version 3420848.
Test Ok

{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:292"},"thread_name":"main","hostname":"forge-e2e-pr-13175-1715300094-6766805bde50fa31553af6217acc2ce63","timestamp":"2024-05-10T00:29:13.059687Z","message":"Deleting namespace forge-e2e-pr-13175: Some(NamespaceStatus { conditions: None, phase: Some(\"Terminating\") })"}
{"level":"INFO","source":{"package":"aptos_forge","file":"testsuite/forge/src/backend/k8s/cluster_helper.rs:400"},"thread_name":"main","hostname":"forge-e2e-pr-13175-1715300094-6766805bde50fa31553af6217acc2ce63","timestamp":"2024-05-10T00:29:13.059707Z","message":"aptos-node resources for Forge removed in namespace: forge-e2e-pr-13175"}

test result: ok. 1 passed; 0 failed; 0 filtered out

Debugging output:

github-actions[bot] avatar May 10 '24 00:05 github-actions[bot]

Forge is running suite realistic_env_max_load on 6766805bde50fa31553af6217acc2ce6358933d7

github-actions[bot] avatar May 10 '24 21:05 github-actions[bot]

:white_check_mark: Forge suite realistic_env_max_load success on 6766805bde50fa31553af6217acc2ce6358933d7

two traffics test: inner traffic : committed: 8167 txn/s, latency: 4802 ms, (p50: 4500 ms, p90: 5700 ms, p99: 10500 ms), latency samples: 3528220
two traffics test : committed: 100 txn/s, latency: 1966 ms, (p50: 1900 ms, p90: 2100 ms, p99: 6300 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.211, avg: 0.207", "QsPosToProposal: max: 0.232, avg: 0.216", "ConsensusProposalToOrdered: max: 0.449, avg: 0.400", "ConsensusOrderedToCommit: max: 0.382, avg: 0.368", "ConsensusProposalToCommit: max: 0.779, avg: 0.769"]
Max round gap was 1 [limit 4] at version 1515184. Max no progress secs was 4.876413 [limit 15] at version 1515184.
Test Ok

github-actions[bot] avatar May 10 '24 21:05 github-actions[bot]