-
Notifications
You must be signed in to change notification settings - Fork 3
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
XRPL tickets and keys rotation examples #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dzmitryhil, @keyleu, @miladz68, and @wojtek-coreum)
a discussion (no related file):
getting this for the second time
forgot to publish in previous review
relayer/client/xrpl/examples/examples_test.go
line 241 at r1 (raw file):
} require.NoError(t, autoFillSignAndSubmitTx(t, remote, &createTicketsTx, senderAccount, senderKey, senderKeySeq)) txRes, err := remote.Tx(*createTicketsTx.GetHash())
nit:
That is type of things which should be fixed in this go ripple lib
This method should definitely have ctx in params & respect it
Also we had issues with hanging requests in sologenic Because they don't use timeouts for their requests
relayer/client/xrpl/examples/examples_test.go
line 335 at r1 (raw file):
require.NoError(t, err) multisigKey := multisigSeed.Key(ecdsaKeyType) multisigKeySeq := lo.ToPtr(uint32(0))
minor:
what does this one mean ?
relayer/client/xrpl/examples/examples_test.go
line 418 at r1 (raw file):
createTicketsWithTicketTx1 := buildCreateTicketsWithTicketTx() require.NoError(t, rippledata.MultiSign(&createTicketsWithTicketTx1, signer1Key, signer1KeySeq, signer1Account))
better abstraction of multisig method could be smth similar to what we do in coreum multisig helper func (c ChainContext) SignAndBroadcastMultisigTx
So instead of passing each signer separately and then combining them within business logic itself we can pass all signers into multisig helper. I think code will look much cleaner then (no combining signatures in code & no magical buildCreateTicketsWithTicketTx
func defined inside another func.
I understand that in coreum SignAndBroadcastMultisigTx
it is a bit easier because we have keyring so we just pass key names but here we need key, sequence and account. IMO we can define struct for these 3 args. And maybe later if we are happy with this method we can propagate it to ripple pkg (rubblelabs).
So final result of the func will be smth like:
type AccForMultisig sturct {
key crypto.Key
sequence *uint32
account Account
}
buildXrpMultisigTx(s MultiSignable, signers ...AccForMultisig) {}
I think this especially improves tests where we have many signers eg. TestMultisigPayment
relayer/client/xrpl/examples/examples_test.go
line 456 at r1 (raw file):
signer1KeySeq := lo.ToPtr(uint32(0)) signer1Account := signer1Seed.AccountId(ecdsaKeyType, signer1KeySeq) t.Logf("Signer1 account: %s", signer1Account)
these pattern is repeated many times
shall we move it to helper ?
Code quote:
signer1Seed, err := rippledata.NewSeedFromAddress(seedPhrase2)
require.NoError(t, err)
signer1Key := signer1Seed.Key(ecdsaKeyType)
signer1KeySeq := lo.ToPtr(uint32(0))
signer1Account := signer1Seed.AccountId(ecdsaKeyType, signer1KeySeq)
t.Logf("Signer1 account: %s", signer1Account)
relayer/client/xrpl/examples/examples_test.go
line 661 at r1 (raw file):
}, }, },
tip:
another interesting side experiment to check could be:
signer_quorum: 1
and 2 accounts with signerWeight of 1
Which should mean that either acc1 OR acc2 should sign
I have never tried this one but AFAIK it should work like this
Code quote:
SignerQuorum: 2, // weighted threshold
SignerEntries: []rippledata.SignerEntry{
{
SignerEntry: rippledata.SignerEntryItem{
Account: &signer1Account,
SignerWeight: lo.ToPtr(uint16(1)),
},
},
{
SignerEntry: rippledata.SignerEntryItem{
Account: &signer2Account,
SignerWeight: lo.ToPtr(uint16(1)),
},
},
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @keyleu, @miladz68, @wojtek-coreum, and @ysv)
a discussion (no related file):
Previously, ysv (Yaroslav Savchuk) wrote…
getting this for the second time
forgot to publish in previous review
Interesting error. T
hat format generated the go after the command go mod init
. Updated.
relayer/client/xrpl/examples/examples_test.go
line 241 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
nit:
That is type of things which should be fixed in this go ripple lib
This method should definitely have ctx in params & respect itAlso we had issues with hanging requests in sologenic Because they don't use timeouts for their requests
Actually I don't plan to use the remote web-socket
at all, and use the RPC instead with the respect of the context and so on.
relayer/client/xrpl/examples/examples_test.go
line 335 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
minor:
what does this one mean ?
This is the account number you retrieve from the family seed. Similar to HD PATH
in cosmos sdk.
relayer/client/xrpl/examples/examples_test.go
line 418 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
better abstraction of multisig method could be smth similar to what we do in coreum multisig helper
func (c ChainContext) SignAndBroadcastMultisigTx
So instead of passing each signer separately and then combining them within business logic itself we can pass all signers into multisig helper. I think code will look much cleaner then (no combining signatures in code & no magical
buildCreateTicketsWithTicketTx
func defined inside another func.I understand that in coreum
SignAndBroadcastMultisigTx
it is a bit easier because we have keyring so we just pass key names but here we need key, sequence and account. IMO we can define struct for these 3 args. And maybe later if we are happy with this method we can propagate it to ripple pkg (rubblelabs).
So final result of the func will be smth like:type AccForMultisig sturct { key crypto.Key sequence *uint32 account Account } buildXrpMultisigTx(s MultiSignable, signers ...AccForMultisig) {}
I think this especially improves tests where we have many signers eg.
TestMultisigPayment
I didn't try to build the fancy client in the examples, just provided a list of functions to verify that we are able to archive what we want. The client will be implemented later during the relayer development, and for sure it will be much simpler in terms of usage.
If you insist I can refactor it in the scope of that PR, but anyways it will be different from the final client.
relayer/client/xrpl/examples/examples_test.go
line 456 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
these pattern is repeated many times
shall we move it to helper ?
Updated.
relayer/client/xrpl/examples/examples_test.go
line 661 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
tip:
another interesting side experiment to check could be:
signer_quorum: 1
and 2 accounts with signerWeight of 1Which should mean that either acc1 OR acc2 should sign
I have never tried this one but AFAIK it should work like this
Tested. Works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @keyleu, @miladz68, and @wojtek-coreum)
relayer/client/xrpl/examples/examples_test.go
line 418 at r1 (raw file):
Previously, dzmitryhil (Dzmitry Hil) wrote…
I didn't try to build the fancy client in the examples, just provided a list of functions to verify that we are able to archive what we want. The client will be implemented later during the relayer development, and for sure it will be much simpler in terms of usage.
If you insist I can refactor it in the scope of that PR, but anyways it will be different from the final client.
Lets refactor
I think the change will make these examples more readable & shorter
Also this will become a decent foundation for final client implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @keyleu, @miladz68, @wojtek-coreum, and @ysv)
relayer/client/xrpl/examples/examples_test.go
line 418 at r1 (raw file):
Previously, ysv (Yaroslav Savchuk) wrote…
Lets refactor
I think the change will make these examples more readable & shorter
Also this will become a decent foundation for final client implementation
Added new Wallet struct and integrated with the examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @keyleu, @miladz68, and @wojtek-coreum)
This change is