-
Notifications
You must be signed in to change notification settings - Fork 35
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: show new events and fetch correctly #650
Conversation
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.
LGTM
Just one coment
scripts/mktcli.js
Outdated
const blockchainEvents = await BlockchainEvent.findByArgs( | ||
'assetId', | ||
const blockchainEvents = await BlockchainEvent.findByAnyArgs( | ||
['assetId', 'landId', '_landId', '_estateId'], |
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.
Also we have _tokenId
for the ERC721 Transfer event
scripts/mktcli.js
Outdated
@@ -290,7 +304,7 @@ const main = { | |||
const asset = await getAssetFromCLIArgs(assetId, assetType) | |||
|
|||
const events = await BlockchainEvent.findByAnyArgs( | |||
['assetId', '_landId'], | |||
['assetId', 'landId', '_landId', '_estateId'], |
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 could add also _tokenId
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 What do you think of using something like:
const argsId = ['assetId', 'landId', '_landId', '_estateId', '_tokenId']
we have the same in two places in the same file
IDK a good name: argsId
, wordToMatchId
, matchId
, argsNames
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.
Agree with abstracting this!
4c1b2cc
to
acfcc4d
Compare
Updated! let me know what you think of the constant name! (POSSIBLE_ASSET_EVENT_IDS) |
It's not trying to be 100% complete with the descriptions