-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[event v2] double emitting in account, coin, fungible asset and object and migrate move-examples #10532
Conversation
@@ -157,6 +157,7 @@ module aptos_framework::object { | |||
} | |||
|
|||
/// Emitted whenever the object's owner field is changed. | |||
#[event] |
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.
note: not sure reusing this struct or create a new one. seems unnecessary to me but a different opinion is welcomed.
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.
go for it...
e4a02ff
to
911b145
Compare
f315ddf
to
bf8bb8c
Compare
831be80
to
bde8dd6
Compare
/// Event emitted when a lock is canceled. | ||
struct CancelLockupEvent has drop, store { | ||
struct CancelLockup has drop, store { |
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.
Is it going to be the convention to drop the suffix "Event"?
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.
My current pattern for aptos-framework is:
- If the current event struct doesn't need add new field to migrate to v2, I will just add
#[event]
attribute directly to that struct to spare the hassle of creating a new struct. - otherwise, create a new struct without
Event
suffix since attribute can tell whether it is an event or not and you cannot share the same event name in the same module.
Does it make sense?
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.
Make sense!
What if the current event v1 struct (for example AbcEvent) needs to add a new field? It will become AbcEventV2 or AbcV2?
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.
there is no way for them to add field. I think any new v2 does not need Event
string in their name.
aptos-move/move-examples/token_objects/ambassador/sources/ambassador.move
Outdated
Show resolved
Hide resolved
83c8378
to
0354ae7
Compare
0354ae7
to
f7f0134
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10532 +/- ##
=======================================
Coverage 69.7% 69.7%
=======================================
Files 2111 2111
Lines 401246 401243 -3
=======================================
+ Hits 279797 279867 +70
+ Misses 121449 121376 -73 ☔ View full report in Codecov by Sentry. |
5c572e6
to
1fcfb55
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1fcfb55
to
80133bf
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.
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.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
❌ Forge suite
|
This reverts commit c7375b8.
This reverts commit c7375b8.
Description
step 1 of #10529
leave other modules for grabs.
Test Plan
ut