feat(sdk-coin-icp): integrate ic0 library for account balance retrieval #6118
Conversation
mohd-kashif
force-pushed
the
WIN-5479
branch
2 times, most recently
from
3904437 to
482a65d
Compare
mohd-kashif
force-pushed
the
WIN-5479
branch
2 times, most recently
from
9bc1c79 to
b2a5acf
Compare
modules/sdk-coin-icp/src/icp.ts
Outdated
| @@ -268,8 +234,7 @@ export class Icp extends BaseCoin { | |||
| const decodedResponse = utils.cborDecode(response.data) as PublicNodeSubmitResponse; | |||
|
|
|||
| if (decodedResponse.status === 'replied') { | |||
| const txnId = this.extractTransactionId(decodedResponse); | |||
| return { txId: txnId }; | |||
| return {}; | |||
There was a problem hiding this comment.
why returning empty object here? We should know what recovery txn hash we generated.
There was a problem hiding this comment.
broadcastTransaction() returns BaseBroadcastTransactionResult which have the txId but we dont have a way to get the txId from the response and we cant change the signature of broadcastTransaction() as its a inherited function
so I have returned a {} response and set the txId in the recover() as we have the builder object in the recover()
There was a problem hiding this comment.
cool, we need to just handle it properly from the caller then.
There was a problem hiding this comment.
nit: add a comment to explain/why status == 'replied' is treated as a success response.
modules/sdk-coin-icp/src/icp.ts
Outdated
| const agent = this.createAgent(); | ||
| const ic = replica(agent, { local: true }); | ||
|
|
||
| const ledger = ic(Principal.fromUint8Array(LEDGER_CANISTER_ID).toText()); |
There was a problem hiding this comment.
I think these steps will be common for any rpc call that we directly make to public fullnode for this canister. For eg v3/submit api and balance fetch api. If yes, then please make a generic function and pass params.
There was a problem hiding this comment.
v3/submit and balance fetch has different process to make the request therefore they have different methods
maybe other methods like feeEstimates or derive might have same process as the balance fetch
There was a problem hiding this comment.
as discussed over call, please add the todo ticket to move the ic-agent calls to a separate file as a follow up.
| @@ -185,6 +185,14 @@ export interface RecoveryOptions { | |||
| memo?: number | BigInt; | |||
| } | |||
|
|
|||
| export interface RecoveryTransaction { | |||
There was a problem hiding this comment.
Is this a standard that other coins also follow for recovery ? As far as I remember they only return txId.
There was a problem hiding this comment.
i checked XRP, TRX and TON coins, they are having similar return fields
There was a problem hiding this comment.
I think the need here is only for id and tx. As discussed offline, pls make the necessary change.
There was a problem hiding this comment.
there is only id and tx
mohd-kashif
force-pushed
the
WIN-5479
branch
2 times, most recently
from
dec516d to
c4cfed7
Compare
mohd-kashif
force-pushed
the
WIN-5479
branch
4 times, most recently
from
9370a10 to
9d8607b
Compare
TIcket: WIN-5479