-
Notifications
You must be signed in to change notification settings - Fork 499
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
txnbuild: SEP 10 challenge builder #1468
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.
I can't formally review this since I authored the PR :) Some questions below.
txnbuild/transaction.go
Outdated
txTimebound := NewInfiniteTimeout() | ||
|
||
if timebound > 0 { | ||
txTimebound = NewTimebounds(time.Now().UTC().Unix(), time.Now().UTC().Unix()+timebound) |
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.
can use the helper method NewTimeout()
here instead
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.
Just following the spec here. The timebound should be {min: now(), max: now() + 300 }
.
NewTimeout
gives {min: 0, max: now() + 300 }
.
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.
oh, that's annoying. Ok, makes sense. Can we make just one call to Now and store in a temporary variable? That will guarantee that the timebound is always the right length.
@bartekn good catch for the network passphrase. We should probably hold off on this until we get more information on that. I see that you opened an issue for it, will track there. |
@poliha this has been clarified in SEP-10 so I think this PR is unblocked. |
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.
LGTM! Just two minor things.
txnbuild/transaction.go
Outdated
} | ||
|
||
// generateRandomString creates a base-64 encoded, cryptographically secure random string of `n` bytes. | ||
func generateRandomString(n int) (string, error) { |
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.
Can this function return []byte
? You need to hex decode this in BuildChallengeTx
.
// tx envelope returned here is ignored because it will be different for each run and cause tests to fail | ||
_, err := BuildChallengeTx(serverSignerSeed, clientAccountID, anchorName, | ||
network.TestNetworkPassphrase, timebound) | ||
check(err) |
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.
I think a simple sanity check (if tx contains manage_data
operation with a correct name) would be great to have.
Shouldn't this be merged into the next minor release branch since it adds a feature? No point reverting it this time, but I just wanted to double check that my understanding is right. |
@tomquisel Yes that would have been the case but we don't have the next minor release branch yet because we just pushed put v1.3. We will base the next release branch off master so this will be in there for sure. |
Implementation for the js-sdk here |
PR Checklist
PR Structure
otherwise).
services/friendbot
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
Summary
Goal and scope
This PR addresses #1466.
Summary of changes
A new factory method
txnbuild.BuildChallengeTx()
returns a SEP 10 compatible "challenge" transaction.Known limitations & issues
This is almost finished, but not quite. It still needs testing, and the envelope currently being produced appears to be invalid according to Stellar Laboratory.
Needs adding to the Changelog. An example of use should be added to the docs.
The code will need to be reworked in v2.0. This is indicated by
Action needed
tags.What shouldn't be reviewed
All to be reviewed.