feat: add token name to show cmd #230
Conversation
|
Minor suggestion: Denomination is also showing Wei also for Eigen token. Also, should we be displaying ETH in ETH(e.g 0.1 ETH) instead of wei for ease of of readability for user? |
yea I need to overhaul and add a common lib for this - so will do that in separate PR. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return bind.NewBoundContract(address, parsed, caller, transactor, filterer), nil |
There was a problem hiding this comment.
nit: return err. Can also use nil, but seeing inconsistent pattern used is why I mention it.
There was a problem hiding this comment.
didn't get this comment?
There was a problem hiding this comment.
It's inconsistent convention. In some places you're reusing err knowing its nil since you've made it past the if-statement check. Other times you are explicitly returning nil.
There was a problem hiding this comment.
sorry I still did not get this. can you give me an example of inconsistency here?
There was a problem hiding this comment.
Haha no worries. When you return, you know err == nil. I saw a place in the code where we return the err object rather than specifying the nil return. Just pointing out we could reconcile these different uses of convention. It's of course a very small detail.
There was a problem hiding this comment.
oh I see what you are saying
return bind.NewBoundContract(address, parsed, caller, transactor, filterer), nil
you want me to return
return bind.NewBoundContract(address, parsed, caller, transactor, filterer), err
I think the first one is the right pattern since I can explicitly see I am returning nil, if I pass err then you have to see the above code to see what is the err.
There was a problem hiding this comment.
can you share where I have returned err when I know it's nil? probably that needs to be changed
There was a problem hiding this comment.
ah I see. stupid generated code. I generally don't do it - let me fix it.
There was a problem hiding this comment.
Agreed on the explicit nil 👍
| return bind.NewBoundContract(address, parsed, caller, transactor, filterer), nil | ||
| } | ||
|
|
||
| func GetTokenName(tokenAddress common.Address, client *ethclient.Client) string { |
There was a problem hiding this comment.
nit: client is a bit nondescript given you're calling something else a ...client.
There was a problem hiding this comment.
ethclient.Client already depicts what this client is so I think it's okay - trying not to be super verbose - unless you have other clients which I think below you see erc20Client
| "Token Address", | ||
| "Amount (Wei)", | ||
| } | ||
| widths := []int{46, 30} | ||
| widths := []int{20, 46, 30} |
There was a problem hiding this comment.
nit: nice to have these as constants to be more self-explanatory of their meaning.
There was a problem hiding this comment.
they are just the width of a columns, I can add that comment but those numbers are arbitrary and doesn't have any particular meaning.
There was a problem hiding this comment.
Totally get that, but leaving them unnamed makes them "magic numbers".
There was a problem hiding this comment.
I guess the only readability improvement it makes is you can know it's tokenname width which is fair.
There was a problem hiding this comment.
Yeah, I usually really like it because it immediately hints at contextual reasoning. For example, if it's a token address, familiarity with max address length. It can be inferred from reading but I find this helps me read smoother
|
Tests please :) |
Fixes # .
What Changed?