-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Smart contract details page hints #327
Conversation
Deployed to https://pr-327-aescan.stg.aepps.com |
@janmichek would it be better to solve conflicts first or can we review it? |
Sure, it was hanging here for a while. Resolved |
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.
Looks pretty decent. I have only a few small notes to take a look at
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.
Thanks for the improvements, it looks spot on!
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.
Once reviewed @janmichek I would suggest to also add @marc0olo for a quick review here. @marc0olo we are approaching the end of this 😃 |
@michele-franchi let me know when I should start checking. not sure if I should already have a look now |
@marc0olo You can go now .) |
src/utils/hints/contractsHints.js
Outdated
eventsCallTransaction: 'Unique identifier of the ContractCallTx transaction which emitted the event.', | ||
eventsCreated: 'Estimated date, time and block height when the smart contract had emitted the event.', | ||
eventsName: 'Name of the emitted event.', | ||
eventsArguments: 'The arguments of the emitted 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.
I guess these are the values (data) emitted for the event? what format is this?
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 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 if I got your question but these are encoded arguments.
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.
to me this is a bit strange displayed. I assume these can be different (encoded) values. I am also not sure if I would replace "arguments" with "values". right now it is not clear if this is only one value or whether these can be different values. or is it one combined encoded value for all possible values/arguments ? 😅
the thing is that it is cutted and can be copied, so the user doesn't really see the value(s). also, they are quite meaningless anyway at this point of time.
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.
Well, I clarified this with Rogerio and I would do the following:
- Get rid of the "Data" column as it's a really specific use case
- Rename arguments column to "Data" to be consistent with Token events
In a separate issue:
- Decode known args (using the parameter aexn-args=true)
- Find a better way to print arguments that cannot be decoded? In my opinion in any case these should be printed somehow and explain to the user what should be done in order to decode them (Until we add contract verification)
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.
@michele-franchi what you mean by to be consistent with Token events
? This issue is about Token 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.
Ah, sorry this is about contract events, got it .) Will fix it
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.
fixed, please check now
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.
Followup #362
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.
To me now it looks good. We still need to adjust contract events but let's handle that in the followup.
Description
resolves #50
Checklist: