TransactionBuilder TxOutContext by dolanbernard · Pull Request #1966 · mobilecoinfoundation/mobilecoin

dolanbernard · 2022-05-12T04:37:03Z

Adding TxOutContext struct to return TxOut, TxOutConfirmationNumber, and a shared secret from transaction builder add_output
Adding iOS native updates to expose TxOutContext to SDK
Adding Android native updates to expose TxOutContext to SDK

Motivation

This is needed for the SDKs to support Moby transaction history

…lly included in its import. Make individual variables in new struct public (may not be optimal fix). Update binding functions in libmobilecoin to use the new context based add_change_output/add_output calls to the transaction builder, and add a new "out" parameter to each of those ffi bindings so the SDK can consume the shared_secret.

…s for shared_secret in the add_change and add_ouput ffi binding functions.

TransactionBuilder add_output returns TxOutContext

transaction/std/src/transaction_builder.rs

iamalwaysuncomfortable

I believe this is too large of a change to transaction builder & that we should probably not be sending the txout shared secret in a struct. Suggested some possible alternatives, would be happy to open up the discussion more.

transaction/std/src/transaction_builder.rs

eranrund · 2022-05-12T18:41:45Z

transaction/std/src/transaction_builder.rs


+/// Transaction output context is produced by add_output method
+/// Used for receipt creation
+#[allow(dead_code)]


Why is this needed?

~~Without that compilation fails due to the shared secret being dead code.~~
Actually it looks like making it public also fixed that. I'll remove it.

transaction/std/src/transaction_builder.rs

cbeck88 · 2022-05-12T21:31:24Z

I think this looks good in concept -- the only thing is, I think this should be the only version of add_output and add_change_output. That will lead to a large diff because there's a lot of code using the transaction builder, but I think that's okay. That's usually what we do when developing the transaction builder -- we allow breaking changes to happen to avoid at all costs having a confusing API

libmobilecoin/include/transaction.h

Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>

…oinfoundation/mobilecoin into tx-builder-shared-secret

remoun · 2022-05-14T01:36:42Z

transaction/std/src/transaction_builder.rs

            transaction_builder.add_input(input_credentials);

-            let (_txout, _confirmation) = transaction_builder
+            let _tx_out_context = transaction_builder


Suggested change

let _tx_out_context = transaction_builder

transaction_builder

remoun · 2022-05-14T01:39:00Z

libmobilecoin/src/transaction.rs

    out_tx_out_confirmation_number: FfiMutPtr<McMutableBuffer>,
+    out_tx_out_shared_secret: FfiMutPtr<McMutableBuffer>,


Since we're switching TransactionBuilder::add_*output to return TxOutContext, would it make sense to similarly make these FFI add_output methods return the whole TxOutContext, replacing these 2 out pointers?

yes, using the second out pointer was my idea, but I think returning a whole TxOutContext would be better.

Ill work on a commit to make this change

cbeck88

LGTM

iamalwaysuncomfortable

Much better, cleared by me!

remoun · 2022-05-16T20:26:56Z

We should merge this PR/branch into master, too

* add_output returns TxOutContext * Overloading modified tx builder functions for compatibility * Add TxOutContext to transaction_std lib.rs so its scope is automatically included in its import. Make individual variables in new struct public (may not be optimal fix). Update binding functions in libmobilecoin to use the new context based add_change_output/add_output calls to the transaction builder, and add a new "out" parameter to each of those ffi bindings so the SDK can consume the shared_secret. * add documentation to TxOutContext variables. Change copy out semantics for shared_secret in the add_change and add_ouput ffi binding functions. * Android Bindings TxOutContext TransactionBuilder add_output returns TxOutContext * Removing comments * Lint * Lint * Fixing test * Fixing test * Remove duplicated add_output and add_change_output * Update libmobilecoin/include/transaction.h Co-authored-by: Remoun Metyas <remoun@mobilecoin.com> * Update libmobilecoin/include/transaction.h Co-authored-by: Remoun Metyas <remoun@mobilecoin.com> * Struct deconstruction to fix build error * Fixing imports * Deconstructing more structs * Fixing syntax * Fixing tests * Remove references to old new functions * Fixing struct deconstruction * Test fix * Test fixes * Imports * Fixing test imports * Lint Co-authored-by: Alex Voloshyn <alexv@mobilecoin.com> Co-authored-by: the-real-adammork <adammork@gmail.com> Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>

Alex Voloshyn and others added 10 commits May 11, 2022 11:44

add_output returns TxOutContext

6542dbc

Overloading modified tx builder functions for compatibility

96bc944

add documentation to TxOutContext variables. Change copy out semantic…

0bf1d79

…s for shared_secret in the add_change and add_ouput ffi binding functions.

Android Bindings TxOutContext

45bca78

TransactionBuilder add_output returns TxOutContext

Removing comments

b2c01bd

Lint

e0acae0

Lint

8112d3f

Fixing test

7080744

Fixing test

7ae9bc6

iamalwaysuncomfortable suggested changes May 12, 2022

View reviewed changes

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

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

iamalwaysuncomfortable requested review from a team, NotGyro, briancorbin, cbeck88, christian-oudard, eranrund, nick-mobilecoin, remoun, the-real-adammork and wjuan-mob May 12, 2022 13:43

iamalwaysuncomfortable approved these changes May 12, 2022

View reviewed changes

iamalwaysuncomfortable suggested changes May 12, 2022

View reviewed changes

iamalwaysuncomfortable reviewed May 12, 2022

View reviewed changes

transaction/std/src/transaction_builder.rs Show resolved Hide resolved

samdealy self-requested a review May 12, 2022 17:49

eranrund reviewed May 12, 2022

View reviewed changes

remoun reviewed May 13, 2022

View reviewed changes

libmobilecoin/include/transaction.h Outdated Show resolved Hide resolved

libmobilecoin/include/transaction.h Outdated Show resolved Hide resolved

dolanbernard and others added 2 commits May 13, 2022 14:59

Remove duplicated add_output and add_change_output

d867ca6

Update libmobilecoin/include/transaction.h

7b83218

Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>

dolanbernard and others added 13 commits May 13, 2022 15:06

Update libmobilecoin/include/transaction.h

de2c2b9

Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>

Struct deconstruction to fix build error

b4f37de

Merge branch 'tx-builder-shared-secret' of https://github.com/mobilec…

162d4fb

…oinfoundation/mobilecoin into tx-builder-shared-secret

Fixing imports

0b12de0

Deconstructing more structs

8946ff2

Fixing syntax

8ac262f

Fixing tests

d88c109

Remove references to old new functions

9559d1b

Fixing struct deconstruction

21a3564

Test fix

9e655f9

Test fixes

e9f5d62

Imports

f204c71

Fixing test imports

652652c

dolanbernard requested review from eranrund, iamalwaysuncomfortable and remoun May 14, 2022 01:09

remoun reviewed May 14, 2022

View reviewed changes

cbeck88 approved these changes May 14, 2022

View reviewed changes

iamalwaysuncomfortable approved these changes May 16, 2022

View reviewed changes

dolanbernard added 2 commits May 16, 2022 12:33

Lint

6f12035

Merge branch 'candidate-1.2' into tx-builder-shared-secret

a76725e

dolanbernard merged commit e4f41d1 into candidate-1.2 May 16, 2022

dolanbernard deleted the tx-builder-shared-secret branch May 16, 2022 20:25

jcape added this to the 1.2.0 Release milestone May 17, 2022

	let _tx_out_context = transaction_builder
	transaction_builder

		out_tx_out_confirmation_number: FfiMutPtr<McMutableBuffer>,
		out_tx_out_shared_secret: FfiMutPtr<McMutableBuffer>,

Conversation

dolanbernard commented May 12, 2022

Motivation

Uh oh!

Uh oh!

Uh oh!

iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eranrund May 12, 2022

Choose a reason for hiding this comment

Uh oh!

dolanbernard May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbeck88 commented May 12, 2022

Uh oh!

Uh oh!

Uh oh!

remoun May 14, 2022

Choose a reason for hiding this comment

Uh oh!

remoun May 14, 2022

Choose a reason for hiding this comment

Uh oh!

the-real-adammork May 14, 2022

Choose a reason for hiding this comment

Uh oh!

the-real-adammork May 14, 2022

Choose a reason for hiding this comment

Uh oh!

cbeck88 left a comment

Choose a reason for hiding this comment

Uh oh!

iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

Uh oh!

remoun commented May 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dolanbernard May 12, 2022 •

edited

Loading