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

feat: transaction status #181

Merged
merged 10 commits into from
Jul 19, 2023
Merged

feat: transaction status #181

merged 10 commits into from
Jul 19, 2023

Conversation

agazso
Copy link
Contributor

@agazso agazso commented Jul 12, 2023

With this PR the Payggy object is able to set the status of the transactions on the backend correctly and update it with messages. It only sends the transaction hash in the messages and the peer in the chat reads the transaction from the blockchain based on that.

What is done:

  • moved balance out of adapter
  • made the Waku and Firebase adapter iterators async to enforce in-order evaluation
    • this was done in order to avoid possible race conditions
  • made the onMessage callback async, no longer returns the updated store
    • instead there is a new updateStore function passed as argument
  • added Sepolia and Chiado testnet configs
    • made Chiado the default
  • made some minor visual fixes in the assets and the dev page
  • added transaction states and the Payggy app correctly displays them (except the error, design is missing for that)

Known issues:

  • Payggy still relies on the messages to arrive for updating the transaction state. If the messages does not arrive then the transaction state won't change. It would be good to add some kind of mechanism to keep track of the active transactions locally once they arrived so that the app does not have to rely on messages.
  • I haven't fixed the Token issue (Refactor Token type so that it does not contain amount field #177) in this PR, but it was very annoying. I suggest removing the amount from the Token type, and creating an Amount type which could have a token and a value property.
  • Chiado RPC endpoints are currently not working. It seems that Nethermind has an issue with ethers@v6. ethers@v5 would work. Workaround was to set up an alternative endpoint running a different execution client. (JSON RPC - Fix differences between Nethermind and Infura (tested by flood) NethermindEth/nethermind#5921 and getTransaction() error ethers-io/ethers.js#3835)
  • Missing error handling all over the place, especially in the design for the Payggy chat widget.
  • Currently there are adapter-like functionalities all over the code (there are Firebase & Waku, balance, transaction and the object adapter). It would be good to consolidate these at some point.
  • There are several onMessage arguments now. It would be better to just have two arguments, one DataMessage and one WakuObjectArgs. I would also rename the latter to WakuObjectContext.
  • During testing it happened to me that the two chat apps were on different blockchain network, but they were connected on the same messaging network. Obviously the transactions did not work. Maybe we could include the chain ID in the messages to avoid similar situations.

@vercel
Copy link

vercel bot commented Jul 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
waku-objects-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 19, 2023 3:48pm

Copy link
Contributor

@vojtechsimetka vojtechsimetka left a comment

Choose a reason for hiding this comment

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

This PR changes the native balance to xDAI for me but the payggy still show ETH as native currency (not sure why) so I can not send funds.

src/lib/adapters/firebase/index.ts Outdated Show resolved Hide resolved
src/lib/objects/payggy/standalone.svelte Show resolved Hide resolved
Comment on lines +22 to +33
token: z.object({
amount: z.string(),
symbol: z.string(),
decimals: z.number().int().positive(),
}),
to: AddressSchema,
from: AddressSchema,
fee: z.object({
amount: z.string(),
symbol: z.string(),
decimals: z.number().int().positive(),
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it as is right now, but later on we should think about whether we don't want to use token address instead of symbol as symbols can be ambiguous (although they should not)

Copy link
Contributor

@vojtechsimetka vojtechsimetka left a comment

Choose a reason for hiding this comment

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

Please rename the initializeBalances to fetchBalances.

I would also love to keep the firebase adapter do everything just in the firebase db (including all transactions etc). I am fine to merge this PR but then I would love to restore the way it originally worked just as we discussed.

src/lib/adapters/firebase/index.ts Outdated Show resolved Hide resolved
Comment on lines +27 to +30
async function _checkBalance(token: Token): Promise<void> {
await checkBalance(address, token)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the object check balances? Shouldn't the app be just checking the balances onload and maybe periodically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because the balance is get through polling and with polling there is always a tradeoff that you either make too many polls and waste resources or too few and then it may be a poor user experience.

I think we should check the balance periodically (let's say once in every minute or so) but if there is a transaction done and it can be expected that changes the balance then it would be good to keep to possibility to force a checkBalance.

@vojtechsimetka vojtechsimetka merged commit fc4283f into main Jul 19, 2023
@vojtechsimetka vojtechsimetka deleted the feat/transaction-status branch July 19, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants