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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #13393 offer shows network's XCH like TXCH on testnet10 #15224

Merged

Conversation

yyolk
Copy link
Contributor

@yyolk yyolk commented May 8, 2023

Purpose:

Fixes #13393

#13393 is currently assigned to @paninaro however I picked it up thinking it was assigned to force the issue to stay open instead of being closed for staleness.
I also thought it'd be a relatively straight forward issue for me to contribute for 馃

This PR is something I roughed in. I'm very open to changing anything about it.

It is rough like:

  • I change print_offer_summary(...) signature
    • reasoning: getting config from load_config in again seemed like an anti-pattern in looking at how the lifecycle of commands would do that once at the top level
  • I use AddressType.XCH.hrp() to get XCH||TXCH
    • reasoning: this seemed like a pretty straight forward method while I was looking for things that were reading the config, which would have the network information, take_offer(...) does already use this for generating the correct address
  • I didn't dive in to see how this is handled elsewhere, to follow that pattern.
  • I used a variable name called network_xch which I am not attached to and think there could be named something better

Current Behavior:

When using chia wallet take_offer on testnet10 the summary will print like:

Summary:
  OFFERED:
    - Testnet Dexie Bucks (Wallet ID: 3): 793.438 (793438 mojos)
  REQUESTED:
    - XCH (Wallet ID: 1): 1.00394 (1003940000000 mojos)

Included Fees: 0.00055 XCH, 550000000 mojos

or when there are royalties like:

Summary:
  OFFERED:
    - 3fb9955bcde745722ce913b74b26f1c18c9efe6c13db1d9ba5fe76e6e41efc31: 0.001 (1 mojo)
  REQUESTED:
    - XCH (Wallet ID: 1): 1e-09 (1000 mojos)

Royalties Summary:
  - For nft187ue2k7duazhyt8fzwm5kfh3cxxfalnvz0d3mxa9lemwdeq7lscstsjpsr:
    - 5E-11 XCH (50 mojos) to txch1nu0dulhuhczyf60upcqml379sq2tggprl4nt92x6unf0m2uc0vpsffqh5d

Total Amounts Requested:
  - 1.05E-9 XCH (1050 mojos)
Included Fees: 0 XCH, 0 mojos

New Behavior:

When using chia wallet take_offer on testnet10 the summary will print like:

Summary:
  OFFERED:
    - Testnet Dexie Bucks (Wallet ID: 3): 793.438 (793438 mojos)
  REQUESTED:
    - TXCH (Wallet ID: 1): 1.00394 (1003940000000 mojos)

Included Fees: 0.00055 TXCH, 550000000 mojos

and with royalties like:

Summary:
  OFFERED:
    - 3fb9955bcde745722ce913b74b26f1c18c9efe6c13db1d9ba5fe76e6e41efc31: 0.001 (1 mojo)
  REQUESTED:
    - TXCH (Wallet ID: 1): 1e-09 (1000 mojos)
Royalties Summary:
  - For nft187ue2k7duazhyt8fzwm5kfh3cxxfalnvz0d3mxa9lemwdeq7lscstsjpsr:
    - 5E-11 TXCH (50 mojos) to txch1nu0dulhuhczyf60upcqml379sq2tggprl4nt92x6unf0m2uc0vpsffqh5d
Total Amounts Requested:
  - 1.05E-9 TXCH (1050 mojos)
Included Fees: 0 TXCH, 0 mojos

Testing Notes:

The extent of my testing was running 2 offers with chia wallet take_offer -e offer...:
One I had from using the tibetswap v2 test, made up of CATs and TXCH
One I pulled from a recent dexie.space testnet NFT trade.

I did not run the test suite.

yyolk added 6 commits May 7, 2023 21:05
- attempts to use `AddressType` with `config` already being loaded
- assumptions made of internal structures being built where the asset type and lookups with literal `XCH`|`xch` can be swapped out 
- value of `txch`|`xch` is set to `network_xch` calling `AddressType.XCH.hrp(config)` 1x
@yyolk yyolk requested a review from a team as a code owner May 8, 2023 03:22
@paninaro paninaro added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label May 8, 2023
@paninaro
Copy link
Contributor

paninaro commented May 9, 2023

Thanks for taking that issue @yyolk! Looks like there's a pre-commit failure caused by the black formatting tool. I think if you run black on the touched file it should reformat correclty.

I tested the code and it works well for me.

@yyolk
Copy link
Contributor Author

yyolk commented May 10, 2023

@paninaro No problem! I've updated the branch after running black. Thank you as well for confirming it's working.

Let me know if you have any other suggestions, if any.

Also, I've become less worried about the variable name of network_xch and changing the signature to a module method print_offer_summary(...) after leaving it sit for a while - however I still defer to you and other reviewers with what would be most appropriate. I don't have a strong opinion one way or the other and I'm still acclimating in small bites every day to the codebase.

Copy link
Contributor

@paninaro paninaro left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again!

@wallentx wallentx merged commit 50c1ea9 into Chia-Network:main May 11, 2023
191 of 192 checks passed
@yyolk yyolk deleted the fix-13393-offer-shows-network-prefix-xch-txch branch May 11, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: Offer shows XCH instead of TXCH when connected to a testnet
3 participants