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

Mobilecoind scis #3212

Merged
merged 9 commits into from
Mar 11, 2023
Merged

Mobilecoind scis #3212

merged 9 commits into from
Mar 11, 2023

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Mar 9, 2023

This adds support for swap proposals and SCIs to mobilecoind.

This is meant to help test the deqs project, which is hard if there is not some wallet software that can incorporate SCIs or make swap proposals.

still to come: incorporating scis into transactions

i also tweaked mob tool because it seemed to hang when i pulled a
new docker image, this tweak makes it display the output.
mob
maybe_run(pull_image_command)
else:
maybe_run_quiet(pull_image_command)
maybe_run(pull_image_command)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jgreat i changed this because, i had a negative user experience when the docker image was revved. The mob prompt command seems to just hang for several minutes without any output. This way, it shows that output. Since the first time the user uses the tool, it's going to do that, I think this is a big improvement

@cbeck88 cbeck88 requested review from a team and NotGyro and removed request for sugargoat and a team March 9, 2023 17:21
@cbeck88 cbeck88 marked this pull request as ready for review March 10, 2023 00:08
mobilecoind/src/payments.rs Show resolved Hide resolved
mobilecoind/src/payments.rs Outdated Show resolved Hide resolved
mobilecoind/src/payments.rs Outdated Show resolved Hide resolved
.get_tx_out_index_by_hash(&utxo.tx_out.hash())?;
log::trace!(logger, "Got global index: {}", global_index);

// Get a ring of mixins and their proofs
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit from excluding the utxo index here when we check whether the utxo is in the ring in build sci?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the code where we check if the utxo is in the ring is a bit janky, IIRC the network requires you to supply a certain number of rings, so in the case that the utxo isn't present and is then added later, you will probably have the wrong ring size and have an invalid transaction.

I'm not 100% sure on that, I took the existing working code from build_transaction.

Copy link
Contributor

@eranrund eranrund left a comment

Choose a reason for hiding this comment

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

Some small nits around comments but other than that LGTM. Thank you!

mobilecoind/api/proto/mobilecoind_api.proto Show resolved Hide resolved
mobilecoind/api/proto/mobilecoind_api.proto Show resolved Hide resolved
mobilecoind/src/payments.rs Show resolved Hide resolved
mobilecoind/src/payments.rs Outdated Show resolved Hide resolved
mobilecoind/src/payments.rs Outdated Show resolved Hide resolved
mobilecoind/src/payments.rs Outdated Show resolved Hide resolved
cbeck88 and others added 3 commits March 10, 2023 14:35
Co-authored-by: wjuan-mob <william@mobilecoin.com>
Co-authored-by: Eran Rundstein <eran@rundste.in>
@cbeck88 cbeck88 enabled auto-merge (squash) March 10, 2023 22:10
@cbeck88 cbeck88 merged commit dd24518 into release/v4.1 Mar 11, 2023
@cbeck88 cbeck88 deleted the mobilecoind-scis branch March 11, 2023 04:15
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.

None yet

3 participants