chore: function refactor, adding of emitting events & improving test coverage #176
Conversation
- Changed capability permission checks in `has_capability_permission` and `destroy_capability` functions to use `cap.trail_id()` instead of `cap.id()`. - Improved test cases for capability creation, revocation, and metadata updates, ensuring proper permission handling and error scenarios. - Added new tests for locking configurations and metadata management, enhancing overall test coverage for the audit trail functionality.
itsyaasir
changed the title
- Reformatted function calls in locking and role tests for better readability by aligning parameters across multiple lines. - Enhanced clarity in test cases related to locking configurations and role permissions, ensuring easier maintenance and understanding of the test logic.
- Fixed typos in comments related to the `Permission` enum and `admin_permissions` function for clarity. - Enhanced documentation to better reflect the intended roles and permissions within the audit trail system.
| public fun issued_capabilities<D: store + copy>(trail: &AuditTrail<D>): &VecSet<ID> { | ||
| &trail.issued_capabilities | ||
| } | ||
|
|
There was a problem hiding this comment.
If we remove these statements we can not use the dot method call syntax anymore.
https://docs.iota.org/developer/advanced/introducing-move-2024#method-syntax
Why do we remove it?
If we want to stay with the dot method call syntax, we also need to maintain the "trail_" prefix (or use a different prefix) for the function implementations.
There was a problem hiding this comment.
If we remove these statements we can not use the dot method call syntax anymore.
We can still use the dot method syntax, a quick example is found here
I don't think the trail_ prefix is necessary, and I removed it because of the "noise" it was producing as well as it is not a move conventions and we don't do this in our products.
There was a problem hiding this comment.
Interesting. The documentation states the following:
You can call any function defined in the same module as the receiver's type as a method if it takes the receiver as its first argument.
For functions defined outside the module, you can declare methods using public use fun and use fun.
When has the " the new direct function names" been introduced? Is it documented somewhere?
The tests run in its own module so the behavior described in the docs is wrong.
There was a problem hiding this comment.
If you check the audit_trail.move file, you will see we are using the dot notation for all the function implementation taking the AuditTrail as it's first argument.
Description of change
This pull request makes significant improvements and refactoring to the
audit_trailMove module, focusing on standardizing naming conventions, adding record deletion functionality, and cleaning up the public API. The changes improve clarity, maintainability, and feature completeness of the audit trail system.Key changes include:
New Features
delete_recordfunction, a correspondingRecordDeletedevent, and adestroydestructor for records. Deletion respects locking configuration and permission checks. [1] [2] [3]Naming and API Refactoring
Renamed many public functions to use concise, consistent names (e.g.,
trail_add_record→add_record,trail_is_record_locked→is_record_locked,trail_get_record→get_record, etc.) and updated internal struct names for clarity (e.g.,TrailImmutableMetadata→ImmutableMetadata). [1] [2] [3] [4] [5] [6] [7]Removed the block of
public usestatements at the end of the module in favor of the new direct function names, simplifying the API surface.Events and Error Handling
AuditTrailDeleted,RecordDeleted) and a new error code for locked records (ERecordLocked). [1] [2] [3]Documentation and Comments
These changes collectively improve the usability, safety, and extensibility of the audit trail module.
Links to any relevant issues
Type of change
How the change has been tested
Change checklist