Arguments parsing fix for Transactions with functions #465
Conversation
bluesign
requested review from
psiemens,
Kay-Zee,
janezpodhostnik and
devbugging
as code owners
Co-authored-by: Gregor Gololicic <75445744+sideninja@users.noreply.github.com>
|
This is so weird, I remember fixing this, but don't understand where it is. |
|
@sideninja I felt deja vu too, I also somehow remember this was fixed. |
|
Might bo some regression at some point, will take a look later, but thank you for fixing this (again). |
| functionDeclaration := sema.FunctionEntryPointDeclaration(program) | ||
| if functionDeclaration != nil { | ||
| if functionDeclaration.ParameterList != nil { | ||
| parameterList = functionDeclaration.ParameterList.Parameters | ||
| } | ||
| } | ||
|
|
||
| transactionDeclaration := program.TransactionDeclarations() | ||
| if len(transactionDeclaration) == 1 { |
There was a problem hiding this comment.
If we are restricting transaction declaration per file (which I agree might be good idea) we should return an error in case there are more which says something like "only one transaction declaration in a source file allowed"
There was a problem hiding this comment.
Actually is it possible to declare two ? Tbh when I was adding, I though it can be 0 (or nil) or 1. So len was the shortest option, but if it can be two declarations, I agree error message can be very nice
There was a problem hiding this comment.
Actually an interesting question, but no matter the answer (which I intend to now find out, although I would say no) you could argue to have multiple of them defined in a file for whatever reason to then only have one of them sent to network, in any case we would need to return an error IMHO whatever the case is right, so the user can now why it didn't work
There was a problem hiding this comment.
I mean the signed transaction can't really contain more than one right, since the signature of payload would then be invalid and AN would also not pass the validation.
There was a problem hiding this comment.
Yeah but if somehow we can define somehow multiple transactions in a file ( like function overloading ) or init overloading, it can be nice to detect with arguments and match the transaction.
For the part about error, isn't this falls in the part of AN responsibility ?
Edit: To expand on this, let's say today network doesn't support, but in the future will allow, I think better to get this error from AN, so in the future we only fix 1 place to allow this change
There was a problem hiding this comment.
I see what you mean. That could be a nice feature, but, I'm not sure we want to promote this kind of patterns, right now we promote having a transactions folder containing transactions in files, and although I think we at the same time shouldn't enforce this, making alternatives possible, it could be too early to allow alternatives before some base patterns are established. This is just my opinion tho. And yes you are right about anyhow AN returning an error but it will lack more context around the error which is available in the CLI, and I like the philosophy of failing early when you can, but also not making requests that will with certainty fail just to make the client a bit lighter.
There was a problem hiding this comment.
Payload will contain all transactions ( if it is allowed )
I don't believe it is, judging from the fact how protobuf converts the message to a transaction, it assumes a single transaction in a message. Assuming from quick glance tho
There was a problem hiding this comment.
I plan to release a new version today or tomorrow and it would be nice to have this included, so if you find the time to just handle an else case for this that would be great :) else let me know I can help with this.
There was a problem hiding this comment.
Hey, I am on vacation right now till the end of the week, would be great if you can add that.
There was a problem hiding this comment.
For sure, enjoy your vacation :)
There was a problem with arguments parsing, causing problems when transactions contain functions.
This PR fixes that, I also changed order of scripts and transactions ( now transactions have priority )