Skip to content
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: marketplace v2 monitor #501

Merged
merged 14 commits into from
Oct 4, 2018
Merged

Conversation

nicosantangelo
Copy link
Contributor

@nicosantangelo nicosantangelo commented Oct 2, 2018

Closes #342 #502

@nicosantangelo nicosantangelo changed the title feat: marketplace v2 monitor [WIP] feat: marketplace v2 monitor Oct 2, 2018
@nicosantangelo nicosantangelo changed the title [WIP] feat: marketplace v2 monitor feat: marketplace v2 monitor Oct 2, 2018
Copy link
Contributor

@nachomazzara nachomazzara left a comment

Choose a reason for hiding this comment

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

Great work nico! Rip (review in progress)

estate.token_id
}" remove land (${x},${y})`
)
const coordinates = Parcel.splitId(parcelId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should change it to const [x, y] = Parcel.splitId(parcelId)

Copy link
Contributor Author

@nicosantangelo nicosantangelo Oct 3, 2018

Choose a reason for hiding this comment

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

Hm, I didn't do that so I can parse the coordinates in the line below this one:
const parcel = { x: Number(coordinates[0]), y: Number(coordinates[1]) }

you think it should look like:

let [x, y] = Parcel.splitId(parcelId)
x = Number(x)
y = Number(y)
const parcel = { x, y }

or

const [x, y] = Parcel.splitId(parcelId)
const parcel = { x: Number(x), y: Number(y) } // I like this one I think

or maybe

const [x, y] = Parcel.splitId(parcelId).map(Number) // I don't like this too much
const parcel = { x, y }

So I can do the same thing everywhere :D

Copy link
Contributor

Choose a reason for hiding this comment

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

I did it!

I prefer The second one

}
},
{ token_id: _estateId }
{ token_id: estate.token_id }
Copy link
Contributor

Choose a reason for hiding this comment

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

If for count we use the the id maybe we should use the id here too or use the token_id for get counts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I didn't see this one, I used .id in the other cases

Copy link
Contributor

@nachomazzara nachomazzara left a comment

Choose a reason for hiding this comment

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

@nicosantangelo finished review

@@ -35,18 +56,17 @@ export async function mortgageReducer(events, event) {
expiresAt,
payableAt,
interestRate,
punitoryInterestRate
punitoryInterestRate,
block_time_created_at
Copy link
Contributor

Choose a reason for hiding this comment

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

Super minor but, We are using blockTime for everything related to block_time_created_at and block_time_updated_at. Is there any reason why we are using a different name here? I mean block_time_created_at instead of blockTime

expires_at: expiresAt,
marketplace_address: marketplace.getAddress(),
asset_type: marketplace.getType(),
block_time_created_at: blockTime,
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to the minor I mentioned

})
])
await Publication.delete({
asset_id: assetId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should delete also by asset_type to avoid undesired deletions

},
{ contract_id }
),
Parcel.update({ owner: winner }, { id: parcelId })
Asset.update({ owner: winner }, { id: assetId })
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are going to have buyer maybe we should check the arg winner or buyer

@@ -14,94 +14,6 @@ export class BlockchainEvent extends Model {
'address'
]
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@nachomazzara nachomazzara merged commit 704431a into master Oct 4, 2018
@nicosantangelo nicosantangelo deleted the feat/estate-publications-monitor branch October 30, 2018 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants