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

Add XRPL payment transaction examples for normal and multi-signing accounts. #2

Merged
merged 3 commits into from
Sep 6, 2023

Conversation

dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Sep 5, 2023

The examples will be used later for the client.


This change is Reviewable

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)


relayer/client/xrpl/examples/examples_test.go line 27 at r1 (raw file):

	ecdsaKeyType = rippledata.ECDSA

	seedPhrase1 = "ssWU9edn2TGCByJAa6CXbAAsCkNzQ" // 0 key : rwPpi6BnAxvvEu75m8GtGtFRDFvMAUuiG3

Aren't these addresses? You pass them to NewSeedFromAddress function


relayer/client/xrpl/examples/examples_test.go line 51 at r1 (raw file):

	t.Logf("Recipient account: %s", recipientAccount)

	// send XRP coins from issuer to recipient (if account is new you need to send 10 XRP to active it)

to activate it

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


relayer/client/xrpl/examples/examples_test.go line 27 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Aren't these addresses? You pass them to NewSeedFromAddress function

That how the function is called :-) Those strings are seeds.


relayer/client/xrpl/examples/examples_test.go line 51 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

to activate it

Done.

wojtek-coreum
wojtek-coreum previously approved these changes Sep 5, 2023
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @ysv)

ysv
ysv previously approved these changes Sep 6, 2023
Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil and @miladz68)


relayer/client/xrpl/examples/examples_test.go line 166 at r2 (raw file):

	// build payment tx using function to prevent signing function mutations
	buildXrpPaymentTx := func() rippledata.Payment {

nit:
any specific reason to keep this func defined inside TestMultisigPayment ?


relayer/client/xrpl/examples/examples_test.go line 256 at r2 (raw file):

	// update base settings
	base := tx.GetBase()
	fee, err := rippledata.NewValue("100", true)

nit:

everyone usually use 12 drops for most txs
But for multisig each extra signature costs 10 drops https://xrpl.org/transaction-cost.html#special-transaction-costs

I think for this examples it is ok to use 100 as hardcoded
But inside bridge txs we should consider it iMO


relayer/client/xrpl/examples/examples_test.go line 263 at r2 (raw file):

}

func submitTx(t *testing.T, remote *ripplewebsockets.Remote, tx rippledata.Transaction) error {

nit: IMO it is reasonable for this func to return txid also
But for example code having it as it is now is ok

@dzmitryhil dzmitryhil dismissed stale reviews from ysv and wojtek-coreum via ad0dc9c September 6, 2023 12:20
Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil and @miladz68)

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68 and @ysv)


relayer/client/xrpl/examples/examples_test.go line 166 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit:
any specific reason to keep this func defined inside TestMultisigPayment ?

Yes, the sign functions mutate the transaction, but we need that same for each singer, that's why we have a function that produces the transaction and prevents the signing of the mutated transaction.


relayer/client/xrpl/examples/examples_test.go line 256 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit:

everyone usually use 12 drops for most txs
But for multisig each extra signature costs 10 drops https://xrpl.org/transaction-cost.html#special-transaction-costs

I think for this examples it is ok to use 100 as hardcoded
But inside bridge txs we should consider it iMO

Right, that is just and example. I added today to must have items to include tx-fees computation. Since it's required to be computed before we sign the tx.


relayer/client/xrpl/examples/examples_test.go line 263 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit: IMO it is reasonable for this func to return txid also
But for example code having it as it is now is ok

If the txid is tx hash - we don't need it, since the tx after the signing already have it.

Copy link
Collaborator

@ysv ysv left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

@dzmitryhil dzmitryhil merged commit 692c28d into master Sep 6, 2023
1 check passed
@dzmitryhil dzmitryhil changed the title Add XRPL examples for Payment transaction for normal and multi-signing accounts. Add XRPL payment transaction examples for normal and multi-signing accounts. Sep 6, 2023
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.

3 participants