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

fixed history_trade and liquidity_pool asset_id field #200

Merged
merged 4 commits into from
Oct 13, 2023
Merged

Conversation

cayod
Copy link
Contributor

@cayod cayod commented Oct 11, 2023

This PR fixes the asset_id field in history_trade and liquidity_pool table.

@cayod cayod requested a review from a team as a code owner October 11, 2023 14:29
Copy link
Contributor

@sydneynotthecity sydneynotthecity left a comment

Choose a reason for hiding this comment

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

The Extract() method for Asset follows a different variable ordering than FarmHashAsset. I think some of the tests are failing due to this change (plus the IDs in the unit test need to be changed.)

I also noticed in the asset.go code that we create an additional hash using the method hashAsset. We return the following output:

return AssetOutput{
		AssetCode:   outputAssetCode,
		AssetIssuer: outputAssetIssuer,
		AssetType:   outputAssetType,
		AssetID:     outputAssetID,
		ID:          farmAssetID,
	}, nil

I think it would make more sense to switch the ID and AssetID to be consistent with naming on other tables:

AssetID:     farmAssetID,
ID:               outputAssetID,

Once the unit tests are passing, I will approve.

internal/transform/liquidity_pool.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sydneynotthecity sydneynotthecity left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the updates

@cayod cayod merged commit 32c5434 into master Oct 13, 2023
2 checks passed
@cayod cayod deleted the fix-farmhash branch October 13, 2023 11:34
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