Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: V2 FungibleToken Standard #77
WIP: V2 FungibleToken Standard #77
Changes from 9 commits
e7f717c
db9cc2d
1718595
03527c0
5bf65e5
61d4243
b39028c
1bb20b6
1ae18bf
7a102a8
32c0010
093318d
079d367
13b5b0d
fccd93c
ba9bf2e
a6fdcb7
90395f4
f461448
ecc811e
306a1fe
ead9c82
e8b7f56
58c157b
c70204c
7aee404
51bfed4
1933db4
8d70279
8c4a3c4
089b9fb
7545b02
4e5f87d
a0d375b
1082bac
0aadea8
5b06fd7
21203f2
5e2328d
84880ef
eb5637a
e6de7ef
ad86e87
81b0e9e
b58c49d
a31fd46
541764b
f24f5f2
50f777c
24763ca
c5fd8b5
54b8066
abe2080
2045a1d
1f32f2e
e70eaaf
b924625
c742c6d
051b966
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Highly Opinionated Views -
Tokens
in events as it is obvious that Fungible token would only emit events related to tokens by adding the prefix; it creates confusion that FT does support besides the tokens.type
can be the first parameter as it is a differentiator among different tokens supported by the contract. It would be cleaner that way, and event filters can use that as a standard to be the first param of the 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.
Are these events coming from the FT contract itself, though? For events, I feel that we want to be as explicit as possible because we do not know what other contracts will name themselves and they will all need to define these events individually (unless that is changing?). For instance, what if
FiatToken
had instead been namedUSDC
? Now the event signature is justWithdrawn
which isn't necessarily as clear anymore. It also helps differentiate from a common event name becauseWithdrawn
is much more likely to be used elsewhere thanTokensWithdrawn
so any indexer trying to make sense of events is going to have less troubleI like this one 🙂
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.
Events should always be standardised within the contract. If some FT is not implementing the standardized events, then those tokens are not compliant. Any other name is not compliant with FT standards. We can't restrict another contract to using the same event name, They can use the
TokensWithdrawn
orWithdrawn
, but the context is automatically different as the contract address differs, so the indexer should not have a problem indexing the events.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.
Not sure I follow, is the event now coming from the FungibleToken address? Otherwise I think I need more info about what you're saying.
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 reason I did it like this was because I wanted to keep the events as similar to their old versions as possible. Adding the argument onto the end would not break off-chain event listener code because it just adds another element to the array instead of replacing an existing element. I also would prefer to keep the same name for the same reason.