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

chore: Use retirement reason instead of TX memo #1822

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

haveanicedavid
Copy link
Contributor

@haveanicedavid haveanicedavid commented Mar 19, 2023

Description

Closes: regen-network/rnd-dev-team#1502

NOTE: regen-network/regen-js#66 will neeed to be merged before this, or you’ll need to pull down that work locally and symlink to properly test - the build will also fail until then. I also was unable to test with a ledger, so am uncertain about amino messages

I’m not super familiar with the code related to most of our marketplace functionality, but believe this should include all of the relevant areas


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...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

(if regen-network/regen-js#66 has’t been merged):

  1. in your local regen-js, pull down the branch from feat: update proto files for regen-ledger v5.0 and cosmos-sdk v0.46.7 regen-js#66 (feat-proto-upgrade-regenv5) & build
  2. Symlink that to your local regen-web setup
  3. test features

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 all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

@haveanicedavid haveanicedavid changed the base branch from main to dev March 19, 2023 15:28
@haveanicedavid haveanicedavid changed the title Chore: Use retirement reason instead of TX memo chore: Use retirement reason instead of TX memo Mar 19, 2023
@haveanicedavid haveanicedavid requested a review from a team March 19, 2023 15:34
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

We need the same changes in useCreditSendSubmit and useCreateBatchSubmit as well

@blushi
Copy link
Member

blushi commented Mar 27, 2023

@haveanicedavid I've just published 0.6.0 for https://www.npmjs.com/package/@regen-network/api?activeTab=readme so you can use it here

@haveanicedavid haveanicedavid requested a review from blushi March 27, 2023 21:28
@blushi blushi requested a review from a team March 28, 2023 07:56
@wgwz
Copy link
Contributor

wgwz commented Mar 29, 2023

tACK - i tested buy w/ retire from a project and the storefront, send w/ retire, retire from my portfolio and retire w/ take from basket. LGTM!

i could see the retirement_reason in the tx's, i.e.:
https://redwood.regen.aneka.io/txs/7433FA8076FAF01539BD87E77F6D06384CD222D5FD83D22D83ACBBC5AEC4C6A3

i was unsure what to test re. changes in, LMK if you'd like me to test something

  • web-registry/src/features/ecocredit/CreateBatchBySteps/hooks/useCreateBatchSubmit.ts

Copy link
Collaborator

@flagrede flagrede left a comment

Choose a reason for hiding this comment

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

tACK

@blushi
Copy link
Member

blushi commented Mar 30, 2023

tACK - i tested buy w/ retire from a project and the storefront, send w/ retire, retire from my portfolio and retire w/ take from basket. LGTM!

i could see the retirement_reason in the tx's, i.e.: https://redwood.regen.aneka.io/txs/7433FA8076FAF01539BD87E77F6D06384CD222D5FD83D22D83ACBBC5AEC4C6A3

i was unsure what to test re. changes in, LMK if you'd like me to test something

  • web-registry/src/features/ecocredit/CreateBatchBySteps/hooks/useCreateBatchSubmit.ts

We have a batch creation form at /ecocredits/create-batch but not sure if that's fully functional yet so ok to skip this I guess

@blushi
Copy link
Member

blushi commented Mar 30, 2023

@erikalogie you can test this on https://deploy-preview-1822--regen-registry.netlify.app/ (from buy, send, retire...)

@erikalogie
Copy link
Collaborator

@blushi this looks good to me!

@blushi blushi merged commit a133b37 into dev Apr 3, 2023
@blushi blushi deleted the chore-1502-retirement-reason branch April 3, 2023 10:40
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.

Use retirement reason instead of tx memo
5 participants