-
Notifications
You must be signed in to change notification settings - Fork 54
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
dapp-feat: Transaction History Support feature complete (#368) #397
Conversation
@@ -78,8 +78,13 @@ describe('getERC20TokenInformation', () => { | |||
|
|||
describe('erc20Mint', () => { | |||
// Mock baseContract object | |||
const txHash = '0x63424020a69bf46a0669f46dd66addba741b9c02d37fab1686428f5209bc759d'; |
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.
nit: make this a class constant so you don't have to create it in each describe/test
const key = localStorage.key(i); | ||
|
||
// only include item with KEY includes 'HEDERA' and NOT include 'READONLY' | ||
if (key?.includes('HEDERA') && !key?.includes('READONLY')) { |
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.
Q: what's the logic here?
Is this filtering out transactions that didn't go through consensus?
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.
That's right. There are some cases in ERC20 or ERC721 where the contract methods are view
functions so I marked them with READONLY
flag. These READONLY
transaction results do not have a txHash
but just the readonly results, and that's why I didn't include it in the /activity page.
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 open a ticket for this. I think it'd be good to capture these actually.
If the format matches the transactions I would say add them in follow up. Just add a column to capture TRANSACTION
/ QUERY
.
If it currently doesn't match we can schedule for later
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.
Ah I see. Unfortunately, the READONLY transactions currently do not have the same structure as other transactions so I'll open a ticket for this
…thods' transaction results Signed-off-by: Logan Nguyen <[email protected]>
…methods' transaction results Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
16de7a4
to
c22f8e3
Compare
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
… (hashgraph#397) * dapp-feat: added transactionType & transactionTimeStamp to all HTS methods' transaction results Signed-off-by: Logan Nguyen <[email protected]> * dapp-feat: added transactionType & transactionTimeStamp to all ERC20 methods' transaction results Signed-off-by: Logan Nguyen <[email protected]> * dapp-feat: transaction history support feature complete Signed-off-by: Logan Nguyen <[email protected]> * dapp-fix: fixed repeated import statements Signed-off-by: Logan Nguyen <[email protected]> * dapp-update: added constants.ts holding multiple shared variables Signed-off-by: Logan Nguyen <[email protected]> * dapp-update: replaced txHash with MOCK_TX_HASH in erc20 tests Signed-off-by: Logan Nguyen <[email protected]> --------- Signed-off-by: Logan Nguyen <[email protected]> Signed-off-by: Mo Shaikjee <[email protected]>
Description:
This PR includes the addition of transactionType and transactionTimeStamp to all HTS and ERC20 methods, in preparation for the /activity page. Additionally, it includes the /activity page to complete the transaction history support feature.
Related issue(s): #368
Fixes #368
** UI Demo **:
Screen.Recording.2023-09-13.at.4.52.11.PM.mov
Notes for reviewer:
Checklist