Skip to content

fix: add proper tx.signAndSend - #123

Merged
chadoh merged 3 commits into
mainfrom
feat/proper-tx-send
Nov 17, 2025
Merged

chadoh merged 3 commits into
mainfrom
feat/proper-tx-send

Conversation

@chadoh

@chadoh chadoh commented Nov 11, 2025 •

Copy link
Copy Markdown
Contributor

The frontend was not actually signing and sending the transaction. Now it is.

to do:

  • WalletButton is not getting updated with new balance without page refresh, despite updateBalance call

@tupui

tupui commented Nov 11, 2025

Copy link
Copy Markdown
Contributor

Indeed!

Maybe we should add a call to simulate and then prepare with the simulation result? I think in some cases that's even a must.

@chadoh

chadoh commented Nov 13, 2025

Copy link
Copy Markdown
Contributor Author

Maybe we should add a call to simulate and then prepare with the simulation result? I think in some cases that's even a must.

You mean just to demonstrate?

It's not needed and seems like overkill in this situation. And I DO think we should show the most common use-case. But maybe we could add it somewhere else! Do you want to open an issue more fully explaining your vision, @tupui?

@chadoh
chadoh requested review from pselle and zachfedor November 13, 2025 15:09
@pselle

pselle commented Nov 13, 2025 •

Copy link
Copy Markdown
Contributor

@chadoh Your PR fails checks, FYI. You might want to rebase on the latest main.

@chadoh
chadoh force-pushed the feat/proper-tx-send branch 2 times, most recently from 111f5de to fc45e99 Compare November 14, 2025 17:00
@chadoh

chadoh commented Nov 14, 2025

Copy link
Copy Markdown
Contributor Author

@tupui @zachfedor @pselle this is passing the build and ready for review! Please read the git commits and provide feedback. My approach to fixing the build may be controversial.

@chadoh
chadoh force-pushed the feat/proper-tx-send branch 3 times, most recently from 9534cdd to 301da09 Compare November 14, 2025 17:26
Comment thread src/contracts/guess_the_number.ts Resolved
Comment thread src/contracts/nft_enumerable_example.ts Resolved
Comment thread src/components/GuessTheNumber.tsx Resolved
Comment thread src/providers/WalletProvider.tsx Resolved
@pselle

pselle commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

I'm cool with this approach (checking in guess_the_number things so they're in git for stub/building purposes), left some comments basically asking for more comments to be added, and perhaps don't add the nft and fungible packages since you don't need them for the purpose of your goals

@zachfedor zachfedor left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I dislike the idea of the stubbed out contract client files.

Scaffold Init now builds the clients, runs npm i, and then runs git init and adds a first commit. So wouldn't a user see immediate changes via git status because the client stub was removed and the real one generated?

EDIT: A bigger problem, now that I'm working in the project, is that any attempt to run scaffold while working in it will modify the checked in stubs and cause extra noise in git output. Not to mention how easy it'll be to accidentally commit the built versions and we'll keep having to revert them to the stub.

@masch

masch commented Nov 14, 2025

Copy link
Copy Markdown

@chadoh Hi! I want to thank you to add this functionality to send signed transactions, which I asked i on #290 issue, and I pulled your branch and tested it on my own contract, which requires signed transactions. It's working perfectly.!
Thank you!

@pselle

pselle commented Nov 17, 2025

Copy link
Copy Markdown
Contributor

@zachfedor the init now running git init can be a factor … but testing through a scaffold init project and starting it (which I did not do last week) -- somehow it's removed the package and/or it's not generating the client? Which would be a bigger issue than the stub, I think. I'll open an issue for this over on the main Scaffold repo.

image

The frontend was not actually signing and sending the transaction. Now
it is.

to do:

- [ ] `WalletButton` is not getting updated with new balance without
  page refresh, despite `updateBalance` call
- remove `useWalletBalance` hook
- add `balances` and `updateBalances` to `useWallet`
- `balances` is a map so we can `balances?.xlm?.balance ?? '-'`
- simplify & improve `useWallet`

  - `signTransaction` defined top-level, always included in props
  - `[state, setState]` removed in favor of flatter & easier-to-reason-about
    state variables such as `[address, setAddress]`

- modify stub in src/contracts/guess_the_number.ts to include all
  GuessTheNumber contract methods

Now when you guess a number, your wallet balance in the top bar updates
as expected, and working with wallet balances feels much more
straightforward.

fixup
@chadoh
chadoh force-pushed the feat/proper-tx-send branch from 301da09 to b49e0b3 Compare November 17, 2025 18:22
@chadoh
chadoh requested a review from zachfedor November 17, 2025 18:23
@chadoh

chadoh commented Nov 17, 2025

Copy link
Copy Markdown
Contributor Author

@zachfedor @pselle this should be good-to-go now!

@chadoh
chadoh dismissed zachfedor’s stale review November 17, 2025 19:05

feedback addressed

@chadoh
chadoh merged commit c858917 into main Nov 17, 2025
2 checks passed
@chadoh
chadoh deleted the feat/proper-tx-send branch November 17, 2025 19:05
@masch

masch commented Nov 18, 2025

Copy link
Copy Markdown

@chadoh and @pselle Thanks for merging this feature! 🙏

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.

5 participants