Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

Address repo updates #134

Merged
merged 6 commits into from
Oct 3, 2019
Merged

Address repo updates #134

merged 6 commits into from
Oct 3, 2019

Conversation

elizabethengelman
Copy link
Contributor

@elizabethengelman elizabethengelman commented Sep 11, 2019 •
edited
Loading

A couple small changes to the address repo that came out of implementing the address table in the transformer repo.

  • Factor out get or create address into one sql string
  • Factor out getChecksumAddress method in address repo
  • Update address repo methods to not need a receiver (1119a7f)
    • I am not entirely sure about this change and would really like some feedback on it. It kind of breaks the pattern we've previously used with repositories, but it seemed overkill to have the address repo methods require receivers that didn't hold any state (i.e. the AddressRepository struct). So this was my first shot at removing that, but I could also see an argument to be made for moving these methods elsewhere like libraries/shared/repository/repository.go since they're a bit different.
    • The reason that the address repo methods didn't don't follow the normal repo pattern is because we needed to use them within other repositories - so each of those repositories would need a AddressRepository passed into it, which seemed like it a lot of extra work just to make sure to stick with a convention.

Once this is released, we'll need to make sure to update transformer repository to use the updated address repo format

@aaizuss
Copy link

aaizuss commented Sep 12, 2019 •
edited
Loading

This seems fine to me - it definitely looks funny to have calls like AddressRepository{}.GetOrCreateAddressInTransaction so it's nice to remove the receiver (any idea how many places we'll need to update the transformers repo?). You say

I could also see an argument to be made for moving these methods elsewhere like libraries/shared/repository/repository.go since they're a bit different.

I think that makes sense if we do end up removing the receiver, but on the other hand, I like having the address stuff in its own file.

Those are my noncommittal thoughts - the rest of the team has worked with this codebase far longer so I'd defer to you all.

@i-norden
Copy link
Collaborator

I agree that it doesn't make since to keep the receiver when none of the methods need one and I think it would make using them inside other repositories cleaner. Maybe as functions they could be lifted into their own address_repository.go file in libraries/shared/repository? If these functions are used by packages outside of vulcanizedb I think it makes sense to move them to libraries/shared.

@elizabethengelman
Copy link
Contributor Author

Thanks for the feedback @i-norden and @aaizuss - I like the idea of moving this to libraries/shared/repository.

@elizabethengelman elizabethengelman mentioned this pull request Sep 19, 2019
Merged
Gslaughl
Gslaughl approved these changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@Gslaughl Gslaughl left a comment

Choose a reason for hiding this comment

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

Nice job, nothing merge-blocking! :shipit:

libraries/shared/repository/address_repository_test.go
@@ -1,5 +1,18 @@
// VulcanizeDB
// Copyright © 2019 Vulcanize
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Double copyright :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must have really wanted to make sure that file was protect. 😆 Nice catch!

pkg/datastore/postgres/repositories/header_sync_log_repository.go Outdated
@@ -22,6 +22,7 @@ import (
"github.com/jmoiron/sqlx"
"github.com/lib/pq"
"github.com/sirupsen/logrus"
repository2 "github.com/vulcanize/vulcanizedb/libraries/shared/repository"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth renaming one of these repositorys?

pkg/datastore/postgres/repositories/full_sync_receipt_repository.go
@@ -84,7 +85,7 @@ func createLogs(logs []core.FullSyncLog, receiptId int64, tx *sqlx.Tx) error {

func (FullSyncReceiptRepository) CreateFullSyncReceiptInTx(blockId int64, receipt core.Receipt, tx *sqlx.Tx) (int64, error) {
var receiptId int64
addressId, getAddressErr := AddressRepository{}.GetOrCreateAddressInTransaction(tx, receipt.ContractAddress)
addressId, getAddressErr := repository.GetOrCreateAddressInTransaction(tx, receipt.ContractAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@elizabethengelman elizabethengelman mentioned this pull request Oct 3, 2019
Merged
rmulhol
rmulhol approved these changes Oct 3, 2019
View reviewed changes
Copy link
Contributor

@rmulhol rmulhol left a comment

Choose a reason for hiding this comment

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

:shipit:

@elizabethengelman elizabethengelman merged commit f6ab938 into staging Oct 3, 2019
@elizabethengelman elizabethengelman deleted the address-repo-updates branch October 3, 2019 16:17
grizz pushed a commit that referenced this pull request Oct 7, 2019
@elizabethengelman @grizz
Address repo updates (#134)
a1e2c55 
* Factor out get or create address into one sql string

* Factor out getChecksumAddress method in address repo

* Update address repo methods to not need a receiver

* Move address repository to libraries/shared
grizz pushed a commit that referenced this pull request Dec 22, 2019
@elizabethengelman @grizz
Address repo updates (#134)
83b8bf8 
* Factor out get or create address into one sql string

* Factor out getChecksumAddress method in address repo

* Update address repo methods to not need a receiver

* Move address repository to libraries/shared
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Gslaughl Gslaughl Gslaughl approved these changes

@rmulhol rmulhol rmulhol approved these changes

@i-norden i-norden Awaiting requested review from i-norden

@aaizuss aaizuss Awaiting requested review from aaizuss

@yaoandrew yaoandrew Awaiting requested review from yaoandrew

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@elizabethengelman @aaizuss @i-norden @rmulhol @Gslaughl