Replace pseudocode CAP-33 examples with real SDK code. #254
Conversation
|
Something went wrong with PR preview build please check |
|
Preview is available here: |
|
Preview is available here: |
|
@rice2000 @vcarl any insights as to why the preview page is rendering the examples as monospace rather than Go code? I believe I followed the format of other examples. |
|
Ah @Shaptic looks like you need |
|
This looks great to me, and I'd love to merge it ASAP, but I can't verify the code myself. @Shaptic do you need a set of eyes on it or have you tested it and verified that it works? |
| sponsorTrustline := []txnbuild.Operation{ | ||
| &txnbuild.BeginSponsoringFutureReserves{ | ||
| SourceAccount: &s1Account, | ||
| SponsoredID: sponsoredAccount, |
There was a problem hiding this comment.
Should sponsoredAccount be aAccount here? If so, the other uses of sponsoredAccount should probably be updated to the corresponding account as well.
There was a problem hiding this comment.
aAccount is the full key-pair, whereas sponsoredAccount is just its public key (you can see sponsoredAccount := aAccount.Address() happen earlier). I figured it was a more descriptive variable name, but would you say it's more confusing since it implies a difference between the two?
There was a problem hiding this comment.
Ah I see. Yeah I was a little confused about what was going on here, but that's my fault for not clearly reading the preamble. I think the example would be easier to understand if you just used A.Address() though. That line already specifies that you're setting the SponsoredID, which is just another way to say sponsored account. You could add a comment to clarify that. What do you think?
There was a problem hiding this comment.
I think you're right. I was trying to figure out why I went with a separate variable in the first place and it's because of this line [L206 if the link doesn't work] (and other revocations). The Account: field should be a *string type, but you can't pass &A.Address() (I'm not sure why, pretty new to Go myself; I think it's because it's an rvalue?).
I think renaming the variable to A := aAccount.Address() will kill two birds with one stone: consolidate all references to the public key and make it clear that it's still Account A, not some newly-introduced sponsored account. Thoughts?
There was a problem hiding this comment.
Yeah that sounds like a good idea!
There was a problem hiding this comment.
Had to use aAddress since A was taken lol but hopefully still gets the point across.
| Line: &assets[2], | ||
| Limit: txnbuild.MaxTrustlineLimit, | ||
| }, | ||
| &txnbuild.EndSponsoringFutureReserves{}, |
There was a problem hiding this comment.
It looks like the transaction source account is aAccount, so the source account here doesn't have to be specified. I noticed in some of your other examples you do specify the source account even if it's equal to the transaction source account. Might be a good idea to keep the usage consistent.
There was a problem hiding this comment.
Good call, I'll consolidate this.
There was a problem hiding this comment.
Addressed, I believe
|
@rice2000 I'd definitely prefer another pair of eyes on it as a sanity check; I pinged the team to take a look at their leisure. |
|
Preview is available here: |
|
Sorry for arriving late. My concern about this type of documentation is, How do we prevent it from biroting? Is there a way to automatically make sure it doesn't? |
Answering to myself, I have created an issue covering this at #257 |
There was a problem hiding this comment.
Looks good to me!
I think it would be helpful to include an example where you need to specify the source account in &txnbuild.EndSponsoringFutureReserves{},
It is helpful for scenarios where someone is creating a sponsored account or assuming all the costs on behalf of the sponsored account.
This adds code samples for the following:
(as with #251, cc @sisuresh and @stellar/horizon-committers)