gnolang / gno

Gno: An interpreted, stack-based Go virtual machine to build succinct and composable apps + Gno.land: a blockchain for timeless code and fair open-source
https://gno.land/
Other
841 stars 342 forks source link

feat(gnokey/maketx): Split SignAndBroadcastTx into 2 independent functions #2384

Closed linhpn99 closed 1 week ago

linhpn99 commented 1 week ago

I want to separate the signing and broadcasting into 2 independent functions to flexibly support creating transactions with or without a signature when running gnokey maketx. Additionally, it also checks if accountNumber or sequence equals 0. If so, it fetches the account from the blockchain to override these values before signing the transaction, in case the user has not set them.

Contributors' checklist... - [ ] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 24.32432% with 28 lines in your changes missing coverage. Please review.

Project coverage is 54.60%. Comparing base (6284669) to head (d0c598e).

Files Patch % Lines
tm2/pkg/crypto/keys/client/maketx.go 0.00% 22 Missing :warning:
tm2/pkg/crypto/keys/client/sign.go 54.54% 4 Missing and 1 partial :warning:
tm2/pkg/crypto/keys/client/send.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2384 +/- ## ========================================== - Coverage 54.64% 54.60% -0.04% ========================================== Files 581 581 Lines 77967 77971 +4 ========================================== - Hits 42602 42576 -26 - Misses 32187 32215 +28 - Partials 3178 3180 +2 ``` | [Flag](https://app.codecov.io/gh/gnolang/gno/pull/2384/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | Coverage Δ | | |---|---|---| | [gno.land](https://app.codecov.io/gh/gnolang/gno/pull/2384/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `61.86% <100.00%> (ø)` | | | [tm2](https://app.codecov.io/gh/gnolang/gno/pull/2384/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang) | `54.41% <17.64%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gnolang#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

linhpn99 commented 1 week ago

Instead of splitting the bundled function, why not export the existing execSign and execBroadcast and use them directly?

Please provide more explanation or an example of your final goal.

Because I'm not sure if making the functions you mentioned public is safe, I choose to split the existing public functions. And this is my usecase when i need this change to make a sponsorable transaction (there is a noopMsg at the beginning of message list and the signature of the sponsoree in signature list) with gnokey maketx send :

...
        msg := bank.MsgSend{
        FromAddress: fromAddr,
        ToAddress:   toAddr,
        Amount:      send,
    }

    // if a sponsor onchain address is specified
    if cfg.RootCfg.Sponsor != "" {
        sponsorAddress, err := crypto.AddressFromBech32(cfg.RootCfg.Sponsor)
        if err != nil {
            return errors.Wrap(err, "invalid sponsor address")
        }

        tx := &std.Tx{
            Msgs:       []std.Msg{vm.NewMsgNoop(sponsorAddress), msg},
            Fee:        std.NewFee(gaswanted, gasfee),
            Signatures: nil,
            Memo:       cfg.RootCfg.Memo,
        }

        err = ExecSign(cfg.RootCfg, args, tx, io)
        if err != nil {
            return err
        }

        if cfg.RootCfg.Broadcast {
            return ExecBroadcast(cfg.RootCfg, tx, io)
        }

        io.Println(string(amino.MustMarshalJSON(tx)))

        return nil
    }

    tx := &std.Tx{
        Msgs:       []std.Msg{msg},
        Fee:        std.NewFee(gaswanted, gasfee),
        Signatures: nil,
        Memo:       cfg.RootCfg.Memo,
    }

    if cfg.RootCfg.Broadcast {
        err := ExecSignAndBroadcast(cfg.RootCfg, args, tx, io)
        if err != nil {
            return err
        }
    } else {
        io.Println(string(amino.MustMarshalJSON(tx)))
    }

...

In this way, I can create a transaction pre-signed by the user, which will serve as input for the sponsor to countersign and execute.

linhpn99 commented 1 week ago

Instead of splitting the bundled function, why not export the existing execSign and execBroadcast and use them directly?

Please provide more explanation or an example of your final goal.

actually i realize that sponsor transaction (transaction with noopMsg) can make and sign individually instead of both making and signing. So that i will close this PR. @moul thanks for review. Now, I'm working on updating the sponsor transaction flow in this PR https://github.com/gnolang/gno/pull/2209 will be open to review when finished. I hope to receive your feedback to improve this feature