-
Notifications
You must be signed in to change notification settings - Fork 37
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
Feature/430 vda xp reward client create #440
base: develop
Are you sure you want to change the base?
Conversation
- Added contract address and abi of `vda-xp-reward`
- Fix typos
* @param to - Recipient wallet address | ||
* @param amount - Token amount to be withdrawn | ||
*/ | ||
public async withdraw( |
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.
Do we need a deposit()
method?
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.
In face, no need deposit()
method.
The owner can deposit tokens by send the VDA token to the address of XPReward
contract.
If we added deposit()
function in the XPReward
contract, the owner should approve the VDA token to the contract before calling the deposit()
function. This involves 2 step for depositing.
I think, transferring VDA token to the XPReward
contract is more conveninent.
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'm proposing adding a deposit()
method to this typescript library, not the smart contract.
A user of this typescript library needs a way to easily deposit tokens.
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.
Added depositToken()
function
@@ -0,0 +1,7 @@ | |||
## These are used for test only. | |||
PRIVATE_KEY = "<Input private key of transaction sender>" | |||
RPC_URL="<Input RPC url for targeting chain>" |
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.
We can remove this now as it should use default RPC URLs
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.
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.
If the RPC_URL is only for the. test script, then make that clear. Add a comment in .env.example
and maybe rename to RPC_URL_TEST_DEPLOY
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.
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.
Looks good, see minor comments.
- Added contract address and abi of `vda-xp-reward`
- Fix typos
- Updated the `XPRewardContract` address and abi
- Update on review - Added `depositToke()` function
…verida/verida-js into feature/430-vda-xp-reward-client-create
Initial version of
vda-xp-reward-client