-
Notifications
You must be signed in to change notification settings - Fork 21
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(frontend): add sol wallet worker #4037
base: main
Are you sure you want to change the base?
Conversation
…alance' into feat(frontend)/add-sol-api-for-balance
…frontend)/add-sol-worker-types
… feat(frontend)/add-sol-scheduler
…rontend)/add-sol-scheduler
…nto feat(frontend)/add-sol-worker # Conflicts: # src/frontend/src/lib/components/loaders/LoaderWallets.svelte
…l-worker # Conflicts: # src/frontend/src/sol/api/solana.api.ts # src/frontend/src/sol/types/network.ts
params: LoadSolWalletParams | ||
): Promise<CertifiedData<bigint | null>> => ({ | ||
data: await loadSolLamportsBalance({ network: params.solanaNetwork, address: params.address }), | ||
certified: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The balance you're fetching here (from 3rd party API) is not certified. Same as with Infura and ETH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ok, what does certified mean? By default the SDK uses commitment level 'finalized', so the transaction is definite. https://solana.com/docs/rpc#default-commitment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
certified mean
Certified as trustworthy or provided by the Internet Computer through a certified query or update call.
Since it comes from a Web2 platform without any verification or certification of validity, it can only be trusted to some extent; hence, certified: false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not review the two ...Wallets.svelte
components. Can you please provide those in a separate PR with their tests.
params: LoadSolWalletParams | ||
): Promise<CertifiedData<bigint | null>> => ({ | ||
data: await loadSolLamportsBalance({ network: params.solanaNetwork, address: params.address }), | ||
certified: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
certified mean
Certified as trustworthy or provided by the Internet Computer through a certified query or update call.
Since it comes from a Web2 platform without any verification or certification of validity, it can only be trusted to some extent; hence, certified: false
.
private loadBalance = async ( | ||
params: LoadSolWalletParams | ||
): Promise<CertifiedData<bigint | null>> => ({ | ||
data: await loadSolLamportsBalance({ network: params.solanaNetwork, address: params.address }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: if you would like to adhere to the coding style, you can destruct the params
//todo implement loading transactions | ||
|
||
this.syncWalletData({ response: { balance } }); | ||
} catch (error: unknown) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double-check, but my initial impression is that you shouldn't use try-catch here because the abstract implementation already handles it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think without this catch there will be no trigger to broadcast the 'syncSolWalletError'
const newBalance = | ||
isNullish(this.store.balance) || | ||
this.store.balance.data !== balance.data || | ||
(!this.store.balance.certified && balance.certified); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should rather crash an error if certified
become true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah sounds reasonable - The btc-wallet.scheduler.ts has the same logic ATM. I'll adjust it here and add a todo there for another PR
this.postMessageWallet({ | ||
wallet: { | ||
balance, | ||
newTransactions: '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It's probably better to fully omit the transactions until those are effectively loaded
- You do no need to provide the certified flag here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Let's keep them - They're not hurting atm and will follow shortly. They are already defined in the schema and I don't want to go back and forth.
- I don't think so, the certified flag is included in the balance and set to false above.
Motivation
We integrate SOL with the same worker approach as btc and ic.
Changes
Implement worker and scheduler.
Tests
Unit tests provided, same as for btc worker.
Additionally:
Bildschirmaufnahme.2024-12-19.um.21.53.30.mov
Note
During the development there were some error messages for mainnet (even though we use alchemy now).
Not reproducable at the moment. Will address this with @StefanBerger-DFINITY to check if the alchemy dashboard shows something.