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

Feature/flexible memo #3269

Open

wjuan-mob wants to merge 20 commits into main from feature/flexible-memo

Contributor

wjuan-mob commented Mar 22, 2023

Add a flexible memo generator option to RTHbuilder
Change set payment intent and set payment request to return a result, which will error when one or the other has already been set.
Add tests for flexible memo generation
Add compute_authenticated_sender_memo and compute_destination_memo

Motivation

This is intended to address issue 3200
This should help provide a way for users to quickly add custom memo types which adhere to the authenticatedsender and destination format while taking advantage of the additional unused 32 bytes.

Future Work

Add tests to sample paykit for the flexible memo generation

wjuan-mob force-pushed the feature/flexible-memo branch from 8a23aba to 0c6e10c Compare

March 22, 2023 19:15

Contributor Author

wjuan-mob commented Mar 22, 2023 •

edited

Loading

Feature/flexible memo #3269 👈
Refactor out a shared compute authenticated sender and computer destination function #3275 : 1 other dependent PR (#3276 )
Update dependencies for cd #3279
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @wjuan-mob and the rest of your teammates on Graphite

wjuan-mob changed the base branch from master to chore/cleanup-memo-handler-for-memo-with-data

March 23, 2023 05:39

wjuan-mob force-pushed the feature/flexible-memo branch from ef0aa0d to f8be8e7 Compare

March 23, 2023 05:39

This was referenced Mar 23, 2023

Clean up match statement and add new memo with data types to sample paykit memo handler #3276

Merged

Refactor out a shared compute authenticated sender and computer destination function #3275

Merged

Base automatically changed from chore/cleanup-memo-handler-for-memo-with-data to feature/add_new_memo_with_data_types

March 23, 2023 07:30

wjuan-mob force-pushed the feature/add_new_memo_with_data_types branch from 544eafc to 962582a Compare

March 23, 2023 19:53

wjuan-mob force-pushed the feature/flexible-memo branch from f8be8e7 to 63c4032 Compare

March 23, 2023 19:53

wjuan-mob force-pushed the feature/add_new_memo_with_data_types branch from 962582a to c5b043b Compare

March 23, 2023 20:49

wjuan-mob force-pushed the feature/flexible-memo branch 3 times, most recently from db24e9e to f78296e Compare

March 23, 2023 21:21

wjuan-mob force-pushed the feature/add_new_memo_with_data_types branch from edf0043 to 5958a24 Compare

March 23, 2023 21:42

wjuan-mob force-pushed the feature/flexible-memo branch from f78296e to 7a67b65 Compare

March 23, 2023 21:42

wjuan-mob mentioned this pull request

Update dependencies for cd #3279

Merged

wjuan-mob marked this pull request as ready for review

March 27, 2023 19:09

wjuan-mob requested review from cbeck88, nick-mobilecoin, dolanbernard, a team, jcape and samdealy and removed request for nick-mobilecoin and a team

March 27, 2023 19:09

Base automatically changed from feature/add_new_memo_with_data_types to master

April 6, 2023 17:02

wjuan-mob force-pushed the feature/flexible-memo branch from 9f8a756 to bed6d74 Compare

April 7, 2023 06:40

jcape assigned wjuan-mob

wjuan-mob force-pushed the feature/flexible-memo branch 2 times, most recently from 2532518 to a91a0fe Compare

April 11, 2023 07:46

wjuan-mob added 3 commits

April 13, 2023 12:45


          lint

c9b5ae3


          Remove todo, add fee check.

6ed5b93

fmt

e9905e0

wjuan-mob force-pushed the feature/flexible-memo branch from f1e6b26 to e9905e0 Compare

April 13, 2023 16:46

samdealy suggested changes

View reviewed changes

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved


          Using trait instead of closures per pr feedback

f453a24

github-actions bot commented Apr 18, 2023

✅ Good job, full-service was built successfully.
Build logs can be found by inspecting the github action run Check that repositories submoduling us will still build after this PR / full-service or by clicking here.

wjuan-mob requested a review from samdealy

April 18, 2023 04:42

samdealy suggested changes

View reviewed changes

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/transaction_builder.rs Outdated Show resolved Hide resolved

cbeck88 reviewed

View reviewed changes

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

wjuan-mob added 2 commits

April 18, 2023 15:29


          Remove unnecessary struct

8c2ffd6


          Remove function based flexible memo generation. Just set the memo pay…

47fd4d1

…loads directly instead

wjuan-mob requested review from samdealy and cbeck88

April 19, 2023 17:06

cbeck88 reviewed

View reviewed changes

transaction/builder/src/memo_builder/rth_memo_builder.rs Show resolved Hide resolved

samdealy approved these changes

View reviewed changes

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

wjuan-mob and others added 4 commits

April 19, 2023 13:54


          Update transaction/builder/src/memo_builder/rth_memo_builder.rs

73a708b

Co-authored-by: Sam Dealy <33067698+samdealy@users.noreply.github.com>


          Update transaction/builder/src/memo_builder/rth_memo_builder.rs

9c2b962

Co-authored-by: Sam Dealy <33067698+samdealy@users.noreply.github.com>


          Update transaction/builder/src/memo_builder/rth_memo_builder.rs

6b63747

Co-authored-by: Sam Dealy <33067698+samdealy@users.noreply.github.com>


          Update transaction/builder/src/memo_builder/rth_memo_builder.rs

7f9bef8

Co-authored-by: Sam Dealy <33067698+samdealy@users.noreply.github.com>

cbeck88 reviewed

View reviewed changes

transaction/builder/src/memo_builder/rth_memo_builder.rs Show resolved Hide resolved

cbeck88 reviewed

View reviewed changes

transaction/builder/src/memo_builder/rth_memo_builder.rs Outdated Show resolved Hide resolved

cbeck88 suggested changes

View reviewed changes

Contributor

cbeck88 left a comment

Remove the custom destination memos, don't allow the caller to set custom first type byte.

Contributor

cbeck88 commented Apr 19, 2023 •

edited

Loading

William has pointed out that this MCIP has paired the payment request id and payment intent id memo types:

https://github.com/mobilecoinfoundation/mcips/blob/main/text/0054-rth-payment-id-memos.md

So that was why we had flexible change memos.

I think we should only allow 0x01 for the authenticated sender memo type bytes, and only 0x02 for the change memo types here, and so restrict the API.

We should also allow defaulting either of the flexible memo payload parameters.

wjuan-mob added 3 commits

April 24, 2023 15:30


          Change error name

ae29bd4


          Addig defaults

205701c


          Changing constants to use named constants

2d92113

wjuan-mob requested a review from cbeck88

April 25, 2023 19:02


          lint

465ddc6

joekottke removed the request for review from jcape

July 5, 2023 18:45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet