Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

Add final rewards+final used fee #75

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

johnnyji-dev
Copy link

No description provided.

- After completing transaction, need to retrieve a final claimed reward and used fee.
@gtg7784 gtg7784 self-requested a review April 9, 2024 11:43
@gtg7784
Copy link
Member

gtg7784 commented May 27, 2024

Hi @johnnyji-dev thank you for your contribution. Can you please add the PR dscription?

@johnnyji-dev
Copy link
Author

Hi @johnnyji-dev thank you for your contribution. Can you please add the PR dscription?

Is there any template for PR description? Is there no format?
Can I just write down description for my PR?
BTW, I checked out my PR and it need to update on "claimedReward" method.

I completed to update and test that method.
I am going to retry to PR.

@gtg7784
Copy link
Member

gtg7784 commented May 29, 2024

Hi @johnnyji-dev thank you for your contribution. Can you please add the PR dscription?

Is there any template for PR description? Is there no format? Can I just write down description for my PR? BTW, I checked out my PR and it need to update on "claimedReward" method.

I completed to update and test that method. I am going to retry to PR.

Hi, you can write summary of updates in this PR without template.

Please update the description and check that claimedReward method (and why it's needed)

thank you for your contribution.

@gtg7784
Copy link
Member

gtg7784 commented May 29, 2024

and also, please run lint before commit your updates. thank you

@johnnyji-dev
Copy link
Author

johnnyji-dev commented May 29, 2024

@gtg7784
[Adding Two Methods]

  • Background : After broadcasting an extrinsic, we need to retrieve "usedFee" or "claimedReward" amount.
  • "usedFee" and "claimedReward" methods are added on "packages/sdk-core/src/modules/dapp-staking/pending-rewards/index.ts".
  • We can get the value by 'extrinsic-hash' and 'block-number'.
  • After stakingV3, There are two types of rewards (bonus and claim).
  • "claimedReward" serves the total(bonus + claim) reward.

It need to merge the commit (77dc1cec6fb9c32a32c7820c0aacc1cecdb08e24).

@gtg7784 gtg7784 requested review from bobo-k2 and removed request for gtg7784 June 9, 2024 18:25
@bobo-k2
Copy link
Contributor

bobo-k2 commented Jun 19, 2024

Hi @johnnyji-dev on more thing. Can you please uplift version in package.json (there are multiple files) from 0.2.9 to 0.3.0

@johnnyji-dev
Copy link
Author

johnnyji-dev commented Jun 23, 2024

Hi @johnnyji-dev on more thing. Can you please uplift version in package.json (there are multiple files) from 0.2.9 to 0.3.0

@bobo-k2
Monosnap package json — astar js  개발 컨테이너: Alpine @ desktop-linux  2024-06-23 16-56-06

I uplifted version in package.json (there are multiple files) to 0.3.0

@bobo-k2 bobo-k2 requested review from impelcrypto and gtg7784 July 8, 2024 06:41
Copy link
Member

@impelcrypto impelcrypto left a comment

Choose a reason for hiding this comment

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

Hi @johnnyji-dev
Thank you for your contribution. Could you help update the version number to 0.3.0 (there are required updating in multiple package.json files)? Thanks in advance.

Ref:
https://github.com/AstarNetwork/astar.js/pull/69/files

let usedFee = BigInt('0');

extrinsicEvents.map((e) => {
if (e.toHuman()?.event?.method == method &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you avoid using toHuman and use polkadot.js types instead. This will also remove need for commaStrToBigInt
The same applies for the method above

@impelcrypto
Copy link
Member

Could you run yarn lint? CI failed in lint and build .

@johnnyji-dev
Copy link
Author

johnnyji-dev commented Jul 8, 2024 via email

Copy link
Contributor

@bobo-k2 bobo-k2 left a comment

Choose a reason for hiding this comment

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

Hi @johnnyji-dev, please address the following

@johnnyji-dev
Copy link
Author

Hi @johnnyji-dev, please address the following

  • Updated method name (claimedReward to getClaimedReward & UsedFee to getUsedFee)
  • Updated README.md file ( add getClaimedReward() & getUsedFee() description. )
  • Add "Dapp Reward" case on getClaimedReward()

@johnnyji-dev johnnyji-dev requested a review from bobo-k2 July 22, 2024 01:12
@bobo-k2
Copy link
Contributor

bobo-k2 commented Jul 22, 2024

Hi @johnnyji-dev, please address the following

  • Updated method name (claimedReward to getClaimedReward & UsedFee to getUsedFee)
  • Updated README.md file ( add getClaimedReward() & getUsedFee() description. )
  • Add "Dapp Reward" case on getClaimedReward()

Hi @johnnyji-dev you forgot the most important point. The code is not building
image

@johnnyji-dev
Copy link
Author

Hi @johnnyji-dev, please address the following

  • Updated method name (claimedReward to getClaimedReward & UsedFee to getUsedFee)
  • Updated README.md file ( add getClaimedReward() & getUsedFee() description. )
  • Add "Dapp Reward" case on getClaimedReward()

Hi @johnnyji-dev you forgot the most important point. The code is not building image

Oh, I missed that part!! And complete to update it.

Copy link
Contributor

@bobo-k2 bobo-k2 left a comment

Choose a reason for hiding this comment

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

Still missing the PR description. Explain reasons for the change

@@ -321,7 +322,7 @@ export const getUsedFee = async (
const blockHash = await api.rpc.chain.getBlockHash(height);
const signedBlock = await api.rpc.chain.getBlock(blockHash);
const apiAt = await api.at(signedBlock.block.header.hash);
const allRecords = (await apiAt.query.system.events()).toArray();
const allRecords: any = await apiAt.query.system.events();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use any Applicable to multiple locations within the code
https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html

Copy link
Author

Choose a reason for hiding this comment

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

@bobo-k2
BTW, Property 'map' does not exist on type 'Codec'.
The type of 'allRecords' is Codec.
I tried to iterate the data by using 'filter' or 'map'. But that make build error(yarn polkadot-exec-tsc --build tsconfig.build.json).

How can I handle this? Can you suggest any reference docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Codec is a base type from which all polkadot.js types are inherited. System.events are of type Vec<FrameSystemEventRecord> so you can change your line to

const allRecords = await api.query.system.events<Vec<FrameSystemEventRecord>>();

Copy link
Author

Choose a reason for hiding this comment

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

When '<Vec>' applied, can not import the member like below error message.
packages/sdk-core/src/modules/dapp-staking/pending-rewards/index.ts:7:10 - error TS2305: Module '"@polkadot/types/lookup"' has no exported member 'FrameSystemEventRecord'.

Alternative, I used the 'EventRecord' and got properties(actualFee, amount) by index.

@johnnyji-dev johnnyji-dev requested a review from bobo-k2 July 29, 2024 09:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants