Feat: add operator to assets #857
Conversation
nachomazzara
force-pushed
the
feat/add-operator-to-assets
branch
from
3ca88df
to
263ed3f
Compare
| token_address: address, | ||
| owner: holder.toLowerCase(), | ||
| operator: operator.toLowerCase() | ||
| }) |
There was a problem hiding this comment.
Maybe we can keep these addresses as lowercase up in line 130 and remove the toLowerCase from insertApproval to keep it consistent
const holder = event.args.operator.toLowerCase()
const operator = event.args.holder.toLowerCase()
const authorized = event.args.authorized === 'true'
try {
log.info(
`[${name}] ${holder} ${
authorized ? 'set' : 'remove'
} ${operator} as approved for all`
)
if (authorized) {
await Approval.insert({ address, holder, operator }) // add actual prop names here
} else {
await Approval.delete({
token_address: address,
owner: holder,
operator
})There was a problem hiding this comment.
The new changes use the toLowercase correctly, but they're still using insertApproval which is not necessary and can be deleted
monitor/reducers/utils.js
Outdated
| @@ -45,3 +47,7 @@ export async function getParcelIdFromEvent(event) { | |||
| const { assetId, landId, _landId, _tokenId } = event.args | |||
| return Parcel.decodeTokenId(assetId || landId || _landId || _tokenId) | |||
| } | |||
|
|
|||
| export function omitParcelId(name) { | |||
| return [eventNames.ApprovalForAll].includes(name) | |||
There was a problem hiding this comment.
Is this future proofing for something? why not:
| return [eventNames.ApprovalForAll].includes(name) | |
| return eventNames.ApprovalForAll === name |
There was a problem hiding this comment.
We can have more events that don't need to check the parcelId as approvalForAll. I wanted to leave it ready for that, the performance is imperceptible in this case.
Also, leaving it as an array, help new coders to quickly know how to add a new event to omit. If they only see a strict equal, they may think that something could break if changed
There was a problem hiding this comment.
Allright. I'm not really an advocate for "leaving it for the future programmer to figure it out", if that's the case I prefer a comment and the correct implementation for the current case (it's not about performance). Let's leave it like this no worries.
On the other hand, the name is not correct (forgot to add it in the first comment, sorry). The method is not omitting anything and there's not connection to a parcelId.
I think maybe shouldOmitEvent makes more sense
There was a problem hiding this comment.
It is not an event omit, it is just an omission of a specific argument of the event.
src/Approval/Approval.model.js
Outdated
| owner, | ||
| operator | ||
| }) | ||
| return res !== undefined |
There was a problem hiding this comment.
Doing a count and checking for 0 instead fo a findOne is way faster
| } | ||
| default: | ||
| throw new Error(`The assetType ${assetType} is invalid`) | ||
| } |
There was a problem hiding this comment.
Two things about this one:
- I think we should use contractAddresses here instead of
env.getas we do in the rest of the codebase...except Estate.modal, but we need to change that one - Not important: You think it's good idea if we add a helper for this? I think we needed it elsewhere
There was a problem hiding this comment.
The helper will work. We should need where we use contractAddresses with a switch by ASSET_TYPE (issue created)
There was a problem hiding this comment.
I should revert it because when we start the server we are not connecting to the eth which is a need for ethereum.js to work
There was a problem hiding this comment.
Ah that's a shame. Maybe we can call loadContracts from the server instead (it doesn't connect to a node). We'll leave it to the other issue
Resolve this convo once you read it! :D
| if (!parcelId && omitParcelId(event.name)) { | ||
| return log.info(`[${name}] Invalid Parcel Id`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Huh I don't get this part.
- If
omitParcelId(event.name)istruethis won't run and,parcelIdwill be undefined below - If
omitParcelId(event.name)isfalseyou don't need to check it again on line 39
There was a problem hiding this comment.
Good catch! I need to remove omitParcelId from the second if
There was a problem hiding this comment.
Still my first point stands, what happens when the first if it's not fulfilled:
if (!omitParcelId(event.name)) { // <==== This one
parcelId = await getParcelIdFromEvent(event)
if (!parcelId) {
return log.info(`[${name}] Invalid Parcel Id`)
}
}
parcelId will be undefined moving forward. Isn't that a problem?
monitor/reducers/estateReducer.js
Outdated
| } ${_operator} as approved for all` | ||
| ) | ||
| if (_approved) { | ||
| await Approval.insertApproval(address, _owner, _operator) |
There was a problem hiding this comment.
check insertApproval comment above
New version of decentraland-eth needed
Depends on decentraland/decentraland-eth#56 and decentraland/decentraland-eth#57
Close #858