Skip to content
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

Merged

Conversation

ByteAtATime
Copy link
Contributor

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):

Before After Notes
old-int new-int new-int-2 To prevent mis-identifications of numbers and to allow the user to copy the value, they can now toggle the number/ether views.
old-tuple new-tuple The tuple now recursively displays each element, with some added quality-of-life details (most notably the array index).
old-struct new-struct Like the tuple, this recursively displays each element, with the key displayed at left.
old-complex-struct new-complex-struct Example of a more complicated item. Notice how the 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

@carletex
Copy link
Member

Hey @ByteAtATime thanks for this, it looks cool!

A few open questions/comments before reviewing the code:

  • The icon might be too big. e.g.
    image

  • Is / the best character to represent the change to ETH? Are we "toggling"? if so, we might want a different icon to to toggle back to wei.

Let's see what others think too!

Thanks again sir!

@ByteAtATime
Copy link
Contributor Author

ByteAtATime commented May 28, 2024

  • The icon might be too big. e.g.

Honestly, it probably is, just wasn't sure what the best practice is considering this is already btn-xs from daisyUI. I'm a little hesitant to override styles manually, but I think that might be the best way?

  • Is / the best character to represent the change to ETH? Are we "toggling"? if so, we might want a different icon to to toggle back to wei.

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?

image

@technophile-04
Copy link
Collaborator

Thanks @ByteAtATime !! looking great ! Added a couple of comments and just pushed a small commit at 37df0ec since there was small bug at TxReceipt component more context here


In DisplayContractVarible part, the nested items inside struct and array are highlighted more as compared to normal variable :

Example :

Screenshot 2024-05-29 at 3 24 51 PM

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

Copy link
Collaborator

@Pabl0cks Pabl0cks left a 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.

image

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

@ByteAtATime
Copy link
Contributor Author

ByteAtATime commented May 29, 2024

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.

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 decimals and everything.

If so, maybe changing the tooltip to "Divide by 1e18" helps a bit?

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!

Or maybe setting the default result to the old one? And letting user multiply.

The problem with this approach is that it might confuse the user that they're receiving a decimal for a uint256 function. (Not sure about this, but I think it might be a problem for new users).

Just thinking out loud! Maybe it's just me that felt it a bit confusing

Nope, it's definitely a problem we need to fix! Never thought of testing it with other contracts haha, great testing!

@technophile-04
Copy link
Collaborator

, with the "Format as ether" tooltip. Some might think we're converting it to ETH (Ξ symbol on result helps with confusion).

Wooah lol didn't thought it but makes sense !!

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?

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.

Or maybe setting the default result to the old one? And letting user multiply.

The problem with this approach is that it might confuse the user that they're receiving a decimal for a uint256 function. (Not sure about this, but I think it might be a problem for new users).

Yeah I agree with this also keeping things a bit raw can make it more intuitive for developer too

@technophile-04
Copy link
Collaborator

Tysm @ByteAtATime !! I think we are almost there just added last couple of comments but its looking great !

@rin-st
Copy link
Collaborator

rin-st commented Jun 2, 2024

Снимок экрана 2024-06-02 в 11 56 23

Noticed that

  1. Color of tooltip in light mode is the same as color of tx result block
  2. Because of overflow-auto sometimes tooltip looks clipped. Not sure hot to fix it easily though. We can add paddings to result but for large tx result with scrolling we still can meet this

@ByteAtATime
Copy link
Contributor Author

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 <NumberDisplay /> has weird issues with the button, is that the only issue that we have right now?

Copy link
Collaborator

@technophile-04 technophile-04 left a 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 🙌

@technophile-04 technophile-04 merged commit 96f6b8a into scaffold-eth:main Jun 11, 2024
1 check passed
zapaz pushed a commit to zapaz/scaffold-eth-fleek that referenced this pull request Jul 3, 2024
Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants