-
Notifications
You must be signed in to change notification settings - Fork 826
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
Better transaction result formatting in debug page #853
Better transaction result formatting in debug page #853
Conversation
Hey @ByteAtATime thanks for this, it looks cool! A few open questions/comments before reviewing the code:
Let's see what others think too! Thanks again sir! |
packages/nextjs/app/debug/_components/contract/utilsDisplay.tsx
Outdated
Show resolved
Hide resolved
Honestly, it probably is, just wasn't sure what the best practice is considering this is already
Hmm, I'm not completely sure where I got that from. I suppose it's to "divide" (by 1e18), but then I guess we would have to replace it with "*" for "multiply by 1e18". I agree with your "toggle", would the "ArrowsRightLeft" icon work better? |
packages/nextjs/app/debug/_components/contract/utilsDisplay.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/app/debug/_components/contract/utilsDisplay.tsx
Outdated
Show resolved
Hide resolved
Thanks @ByteAtATime !! looking great ! Added a couple of comments and just pushed a small commit at 37df0ec since there was small bug at In DisplayContractVarible part, the nested items inside struct and array are highlighted more as compared to normal variable : Test code :// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;
contract YourContract {
struct SimpleStruct {
uint256 val;
uint256 secondVal;
bool flag;
address addr;
}
struct ComplexStruct {
uint256 val;
bytes data;
uint256[5] arr;
}
uint256[10] public arr = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
address public owner = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
address public owner2 = 0x67cE05a68887E1a5fD4F208d3f7F7eC66aCc8534;
function simpleArray() public view returns (uint256[10] memory) {
return arr;
}
function updateOwner(address _owner) public {
owner = _owner;
}
function getAddr(address _dummyAddress) public pure returns (address) {
return _dummyAddress;
}
function simpleTuple(uint256 _val) public pure returns (uint256, uint256) {
return (_val, _val * 2);
}
function OnlyDiplaySimpleStruct()
public
pure
returns (SimpleStruct memory)
{
return
SimpleStruct(
5,
5 * 2,
true,
0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
);
}
function simpleStruct(
uint256 _val
) public pure returns (SimpleStruct memory) {
return
SimpleStruct(
_val,
_val * 2,
true,
0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
);
}
function simpleInt(uint256 _val) public pure returns (uint256) {
return _val;
}
function simpleUint256Array(
uint256 _val
) public pure returns (uint256[5] memory) {
return [_val, _val * 2 ether, _val * 3 ether, _val * 4, _val * 5];
}
function complexStruct(
uint256 _val
) public pure returns (ComplexStruct memory) {
return
ComplexStruct(
_val,
abi.encodePacked(_val),
[_val, _val * 2, _val * 3, _val * 4, _val * 5]
);
}
}
if its get too tedious to handle this we could also have another PR which focuses on styling and maybe also have accordian to display complex variables |
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.
Nice improvements! GJ 🙌
UX nitpick when you check the balance of an ERC20.
Now we're showing the int value, with the "Format as ether" tooltip. Some might think we're converting it to ETH (Ξ symbol on result helps with confusion).
Calculating the symbol of the ERC20 to show it on the tooltip + result may be a good solution, but probably would make the code messy. If so, maybe changing the tooltip to "Divide by 1e18" helps a bit?
Or maybe setting the default result to the old one? And letting user multiply.
Just thinking out loud! Maybe it's just me that felt it a bit confusing
Well, the thing is that the contract is not passed into the function, and even if it were we would also need to load the
I think this might be the best approach, I will add a commit. If anyone else has any ideas, I'd be happy to hear them!
The problem with this approach is that it might confuse the user that they're receiving a decimal for a
Nope, it's definitely a problem we need to fix! Never thought of testing it with other contracts haha, great testing! |
Wooah lol didn't thought it but makes sense !!
Umm ERC20 symbol will be adding complexity and I think its not worth it that much (sound a great feature for Abi.Ninja though). Like "Divide by 1e18" idea! I think for now it is best approach.
Yeah I agree with this also keeping things a bit raw can make it more intuitive for developer too |
packages/nextjs/app/debug/_components/contract/utilsDisplay.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/app/debug/_components/contract/utilsDisplay.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/app/debug/_components/contract/utilsDisplay.tsx
Outdated
Show resolved
Hide resolved
Tysm @ByteAtATime !! I think we are almost there just added last couple of comments but its looking great ! |
packages/nextjs/app/debug/_components/contract/utilsDisplay.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/app/debug/_components/contract/ReadOnlyFunctionForm.tsx
Outdated
Show resolved
Hide resolved
Oh my god... I'd forgotten about this PR! Thanks @technophile-04 for all the commits and fixes! What are the main issues that need to be fixed? I see that the |
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 see that the has weird issues with the button,
You mean to say the alignment? already fixed at 6d168a2
Maybe we can do some more touchup's to UI in separate PR
Thanks @ByteAtATime for the PR and other for review and suggestions, Merging this 🙌
Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com>
Description
This PR improves the formatting of the transaction result in the debug page, especially for numbers, tuples, and structs.
Here is a table comparing the changes (click on images to enlarge):
arr
field is nicely indented.The contract used in these examples can be found here.
Additional Information
I would consider this to be a "straightforward" change, so have not filed an issue.
Your ENS/address:
byteatatime.eth