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

[ioctl] build xrc20 transfer from command line into new ioctl #3444

Merged
merged 35 commits into from
Sep 12, 2022

Conversation

saitofun
Copy link
Contributor

@saitofun saitofun commented Jun 11, 2022

Description

[ioctl] build xrc20 transfer from command line into new ioctl

Fixes #3339

Type of change

Please delete options that are not relevant.

  • Code refactor or improvement

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • make test

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@saitofun saitofun requested a review from a team as a code owner June 11, 2022 07:52
@saitofun saitofun requested a review from huof6829 June 11, 2022 07:53
@codecov
Copy link

codecov bot commented Jun 11, 2022

Codecov Report

Merging #3444 (9cbed1b) into master (a20e489) will decrease coverage by 0.55%.
The diff coverage is 79.49%.

@@            Coverage Diff             @@
##           master    #3444      +/-   ##
==========================================
- Coverage   75.43%   74.87%   -0.56%     
==========================================
  Files         247      267      +20     
  Lines       22845    23753     +908     
==========================================
+ Hits        17233    17786     +553     
- Misses       4685     5042     +357     
+ Partials      927      925       -2     
Impacted Files Coverage Δ
action/action_deserializer.go 57.14% <ø> (ø)
action/protocol/poll/nativestaking.go 41.08% <0.00%> (-0.65%) ⬇️
action/protocol/poll/staking_command.go 10.71% <0.00%> (ø)
action/protocol/staking/read_state.go 15.38% <0.00%> (ø)
action/protocol/vote/probationlist.go 87.50% <ø> (ø)
api/blocklistener.go 70.73% <0.00%> (ø)
api/websocket.go 5.17% <0.00%> (-0.19%) ⬇️
blockchain/block/block_deserializer.go 71.15% <ø> (ø)
blockchain/blockchain.go 0.89% <0.00%> (ø)
blockchain/filedao/filedao_legacy.go 85.80% <ø> (ø)
... and 116 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines 17 to 18
"github.com/iotexproject/iotex-core/ioctl"
"github.com/iotexproject/iotex-core/ioctl/flag"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iotex-core should be a single group. put line17~18 to line36

ioctl/client.go Outdated
@@ -154,6 +156,11 @@ func (c *client) SelectTranslation(trls map[config.Language]string) (string, con
return trl, config.English
}

func (c *client) SelectTranslationText(trls map[config.Language]string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need

ioctl/newcmd/action/xrc20transferfrom.go Show resolved Hide resolved
Comment on lines 38 to 39
Use: config.TranslateInLang(_xrc20TransferFromCmdUses, config.UILanguage),
Short: config.TranslateInLang(_xrc20TransferFromCmdShorts, config.UILanguage),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use client.SelectTranslation()

return errors.Wrap(err, "cannot generate bytecode from given command")
}
err = Execute(client, cmd, contract.String(), big.NewInt(0), bytecode)
return output.PrintError(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don's use output.

Comment on lines 248 to 253
if addressOrAlias == "" {
addressOrAlias, err = config.GetContextAddressOrAlias()
if err != nil {
return
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are done in line 254,

"math/big"
"strconv"

"github.com/iotexproject/iotex-core/ioctl"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put iotex-core together

Comment on lines 16 to 19
"github.com/pkg/errors"
"github.com/spf13/cobra"

"github.com/iotexproject/iotex-address/address"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are another single group


"github.com/iotexproject/iotex-address/address"

"github.com/iotexproject/iotex-core/ioctl/cmd/alias"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use iotex-core/ioctl/cmd/, use iotex-core/ioctl/newcmd/

"math/big"

"github.com/iotexproject/iotex-core/ioctl"
"github.com/iotexproject/iotex-core/ioctl/cmd/alias"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use iotex-core/ioctl/cmd/, use iotex-core/ioctl/newcmd/

}
]`

var _xrc20ABI abi.ABI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put global variable into client.

if err != nil {
return errors.Wrap(err, "failed to parse amount")
}
bytecode, err := _xrc20ABI.Pack("transferFrom", owner, recipient, amount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put line59 into client and use like client.PackABI

@huof6829
Copy link
Contributor

huof6829 commented Jun 13, 2022

Pls create _test.go and add unit test.

URL string `json:"url"`
}

func (m *sendMessage) String() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PackABI is used in some commands. client warps the common operations. command can use client's method to do own business.

)

// PackABI pack abi with abi content, pack name and arguments
func PackABI(content string, name string, args ...interface{}) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not global. should be client's method

// ABI JSON content

var (
XRC20ABI = `[{"constant":false,"inputs":[{"name":"spender","type":"address"},{"name":"value","type":"uint256"}],"name":"approve","outputs":[{"name":"","type":"bool"}],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"totalSupply","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"name":"from","type":"address"},{"name":"to","type":"address"},{"name":"value","type":"uint256"}],"name":"transferFrom","outputs":[{"name":"","type":"bool"}],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[{"name":"who","type":"address"}],"name":"balanceOf","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[{"name":"to","type":"address"},{"name":"value","type":"uint256"}],"name":"transfer","outputs":[{"name":"","type":"bool"}],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[{"name":"owner","type":"address"},{"name":"spender","type":"address"}],"name":"allowance","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"anonymous":false,"inputs":[{"indexed":true,"name":"owner","type":"address"},{"indexed":true,"name":"spender","type":"address"},{"indexed":false,"name":"value","type":"uint256"}],"name":"Approval","type":"event"},{"anonymous":false,"inputs":[{"indexed":true,"name":"from","type":"address"},{"indexed":true,"name":"to","type":"address"},{"indexed":false,"name":"value","type":"uint256"}],"name":"Transfer","type":"event"}]`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not global. should be client's member.

if err != nil {
return errors.Wrap(err, "failed to parse amount")
}
bytecode, err := ioctl.PackABI(ioctl.XRC20ABI, "transferFrom", owner, recipient, amount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client.packABI("transferFrom", owner, recipient, amount)

@huof6829
Copy link
Contributor

@saitofun fix ci Fail

@huof6829
Copy link
Contributor

@saitofun I fixed, pls check it. Thx

@huof6829
Copy link
Contributor

@saitofun action_test.go is handling by #3472. you can refer to it.

LuckyPigeon added a commit to LuckyPigeon/iotex-core that referenced this pull request Jun 29, 2022
@saitofun saitofun requested review from huof6829 and Liuhaai July 6, 2022 16:03
@saitofun
Copy link
Contributor Author

saitofun commented Jul 8, 2022

xrc20transferfrom_test.go:98 run failed

already fix it

@huof6829
Copy link
Contributor

after #3472 merged, this pr continue. because the same file action.go

@LuckyPigeon LuckyPigeon mentioned this pull request Aug 4, 2022
11 tasks
ioctl/client.go Outdated
Comment on lines 135 to 138
xrc20ABI, err := abi.JSON(strings.NewReader(_xrc20ABI))
if err != nil {
return nil, errors.Wrap(err, "cannot get abi JSON data")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may put it into init, and panic if any error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

ioctl/client.go Outdated
@@ -397,6 +426,10 @@ func (c *client) WriteHdWalletConfigFile(mnemonic string, password string) error
return nil
}

func (c *client) PackABI(name string, args ...interface{}) ([]byte, error) {
return c.xrc20ABI.Pack(name, args...)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xrc20abi is more like a const, instead of a parameter of client

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xrc20ABI.Pack(name, args...) is used in other xrc20 commands: xrc20_allowance、 xrc20_approve ....
so make this function.
It seems that xrc20abi has nothing to do with client. If so, I will put it into package of xrc20.


client.SetEndpointWithFlag(cmd.PersistentFlags().StringVar)
client.SetInsecureWithFlag(cmd.PersistentFlags().BoolVar)
client.SetXrc20ContractAddrWithFlag(cmd.PersistentFlags().StringVarP, cmd.MarkFlagRequired)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xrc20 contract address is not a client feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok,

@LuckyPigeon LuckyPigeon mentioned this pull request Sep 7, 2022
11 tasks
@sonarcloud
Copy link

sonarcloud bot commented Sep 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Liuhaai
Copy link
Member

Liuhaai commented Sep 9, 2022

@huof6829 could you help fix the ci failure?

@LuckyPigeon LuckyPigeon mentioned this pull request Sep 10, 2022
11 tasks
@huof6829 huof6829 merged commit 5e620b3 into master Sep 12, 2022
@dustinxie dustinxie deleted the issue3339 branch December 17, 2022 04:08
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.

[ioctl] build xrc20 transfer from command line into new ioctl
4 participants