feat: estimate transaction cost #399
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
vojtechsimetka
force-pushed
the
feat/estimate-transaction-cost
branch
from
a211c1c to
08905ce
Compare
| } | ||
| } | ||
| let fee: bigint | undefined = undefined | ||
| let feeError: undefined | Error = undefined |
There was a problem hiding this comment.
feeError is not set anywhere. Maybe on line 51 it could be set?
There was a problem hiding this comment.
good catch! Forgot to commit this one, thanks!
| import { defaultBlockchainNetwork } from '$lib/adapters/transaction' | ||
| import type { Balance } from './schemas' | ||
| import type { Splitter, SplitterFactory } from './contracts/types' | ||
|
|
||
| const splitterFactoryAddress = defaultBlockchainNetwork.objects.splitterFactory |
There was a problem hiding this comment.
I just realized that the splitterFactory address is hardcoded in the BlockchainNetwork type. I don't think that's a good place to store it, it seems it would be better if the object code would define it, especially if the object is an external type then there is no way that it can expect the host application to know about it and expose it in the SDK.
This does not belong in this PR, but I just wanted to document this issue somewhere.
There was a problem hiding this comment.
I kinda agree. At the same time, it also depends on which blockchain the app is connected to. I'll create an issue about this but I believe there is some discussion needed.
There was a problem hiding this comment.
The code looks good, but please check out my comments.
I tested with Payggy where there is a field called Transaction fee (max) which looks reasonable. Then after the transaction is done the same is displayed but at that time the exact amount can be known and it would be better to display that instead of the estimated maximum which has no relevance anymore.
resolves #314