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.

Sign up for GitHub

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

Jump to bottom

refactor(position): Extract JSON From Position #495

Merged
merged 17 commits into from
Feb 5, 2025
Merged

refactor(position): Extract JSON From Position #495

merged 17 commits into from
Feb 5, 2025

Conversation

notJoon
Copy link
Member

@notJoon notJoon commented Feb 3, 2025 •
edited
Loading

Description

  • Extracted the JSON generation part within the function
  • tokenID -> positionID (partial changes)

related with #473

@notJoon notJoon marked this pull request as ready for review February 3, 2025 09:07
@notJoon notJoon requested review from onlyhyde and r3v4s February 3, 2025 09:08
r3v4s
r3v4s reviewed Feb 3, 2025
View reviewed changes
Copy link
Member

@r3v4s r3v4s left a comment

Choose a reason for hiding this comment

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

Seems like naming is mixed, what's differnene?

lpPositionId
positionId

@notJoon
Copy link
Member Author

notJoon commented Feb 3, 2025

Seems like naming is mixed, what's differnene?

lpPositionId positionId

Nothing. modified it with control+F, so it must have gotten mixed up.

dongwon8247
dongwon8247 requested changes Feb 3, 2025
View reviewed changes
Copy link
Member

@dongwon8247 dongwon8247 left a comment

Choose a reason for hiding this comment

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

Change all tokenID to positionID.
lpPositionId to positionID too.

onlyhyde
onlyhyde previously approved these changes Feb 4, 2025
View reviewed changes
Copy link
Member

@onlyhyde onlyhyde left a comment

Choose a reason for hiding this comment

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

LGTM

dongwon8247
dongwon8247 requested changes Feb 4, 2025
View reviewed changes
Copy link
Member

@dongwon8247 dongwon8247 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@notJoon position ID -> positionID, lpPositionId -> PositionId.

Do not request a review until you apply the requested changes to all parts.

contract/r/gnoswap/position/_helper_test.gno Outdated Show resolved Hide resolved
contract/r/gnoswap/position/position_test.gno Show resolved Hide resolved
contract/r/gnoswap/position/position_test.gno Show resolved Hide resolved
contract/r/gnoswap/position/position.gno Show resolved Hide resolved
contract/r/gnoswap/position/position.gno Show resolved Hide resolved
contract/r/gnoswap/position/position.gno Show resolved Hide resolved
contract/r/gnoswap/position/position.gno Show resolved Hide resolved
@notJoon notJoon mentioned this pull request Feb 5, 2025
Draft
mconcat
mconcat previously approved these changes Feb 5, 2025
View reviewed changes
@notJoon
Copy link
Member Author

notJoon commented Feb 5, 2025

@dongwon8247

All comments except the Position ID have been resolved in this commit.

Please note that while variable names should be written without spaces, I didn't reflect that change for comments because they're easier to read with spaces. Variables should be written without spaces, but when text serves as a comment, spaces make it more readable

b2ea39a

dongwon8247
dongwon8247 previously approved these changes Feb 5, 2025
View reviewed changes
onlyhyde
onlyhyde previously approved these changes Feb 5, 2025
View reviewed changes
@notJoon notJoon dismissed stale reviews from onlyhyde and dongwon8247 via a6a7b11 February 5, 2025 09:32
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Feb 5, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
18.8% Duplication on New Code

See analysis details on SonarQube Cloud

@notJoon notJoon requested a review from onlyhyde February 5, 2025 09:41
@onlyhyde onlyhyde merged commit 275bda4 into main Feb 5, 2025
18 of 19 checks passed
@onlyhyde onlyhyde deleted the change-positionID branch February 5, 2025 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onlyhyde onlyhyde onlyhyde approved these changes

@mconcat mconcat mconcat left review comments

@dongwon8247 dongwon8247 dongwon8247 left review comments

@r3v4s r3v4s Awaiting requested review from r3v4s

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

Successfully merging this pull request may close these issues.
5 participants
@notJoon @onlyhyde @mconcat @dongwon8247 @r3v4s