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

refactor(x/ecocredit): consistent address field names #1153

Merged
merged 10 commits into from
Jun 3, 2022

Conversation

ryanchristo
Copy link
Member

@ryanchristo ryanchristo commented Jun 2, 2022

Description

Closes: #1080

This pull requests makes the following changes to field names:

  • All previous references to owner in relation to sell orders have been renamed to seller (previously we were using owner and seller but we are using buyer for buy orders and owner for credits)
  • All previous references to account for credit balances have been renamed to address (previously we were using account and address but address is more in line with the sdk bank module)
  • All previous references of holder in relation to credits being cancelled, retired, and bridged have been renamed to owner (previously we were using holder and owner)

This pull request also updates SellOrdersByAddress to SellOrdersBySeller.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@ryanchristo ryanchristo changed the title refactor(x/ecocredit): consistent ecocredit address field names refactor(x/ecocredit): consistent address field names Jun 2, 2022
@ryanchristo ryanchristo requested a review from clevinson June 2, 2022 20:55
@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #1153 (92760d5) into master (f5c7cd9) will decrease coverage by 0.01%.
The diff coverage is 73.17%.

@@            Coverage Diff             @@
##           master    #1153      +/-   ##
==========================================
- Coverage   69.36%   69.34%   -0.02%     
==========================================
  Files         220      220              
  Lines       22798    22798              
==========================================
- Hits        15814    15810       -4     
- Misses       5602     5606       +4     
  Partials     1382     1382              
Flag Coverage Δ
experimental-codecov 69.39% <73.17%> (+0.07%) ⬆️
stable-codecov 63.64% <71.54%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@clevinson
Copy link
Member

Generally giving a concept ACK on this. I do think it might be better to try and stay consistent with bank module usage of address where appropriate.

So I think:

  • QueryBalanceReuqest
  • QueryBalancesRequest
  • BatchBalanceInfo
  • BatchBalance

Might be better to keep as "address" instead of "owner".

@ryanchristo ryanchristo marked this pull request as ready for review June 3, 2022 00:55
@@ -20,7 +20,7 @@ message SellOrder {
// id is the unique ID of sell order.
uint64 id = 1;

// seller is the bytes address of the owner of the credits being sold.
// seller is the address of the account that is selling credits.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// seller is the address of the account that is selling credits.
// seller is the address (in bytes) of the account that is selling credits.

maybe it still makes sense to explicitly state that its in byte representation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's necessary. I previously made a change to add something like "unique string identifier" in relation to credit class IDs and it was pointed out that this is excessive because the field clearly states the type. I would have agreed before but I don't think we need to state the type unless we are adding details such as "any arbitrary string".

@technicallyty technicallyty self-requested a review June 3, 2022 15:30
Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

👍🏻

@ryanchristo ryanchristo merged commit 2151ad1 into master Jun 3, 2022
@ryanchristo ryanchristo deleted the ryan/1080-address-fields branch June 3, 2022 22:01
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.

Inconsistent address fields in ecocredit proto definitions
3 participants