Paymaster PR 2: add paymaster_buildTransaction method + tests
#801
Conversation
thiagodeev
changed the title
paymaster_buildTransaction method + tests
There was a problem hiding this comment.
Pre-liminary review. The code looks correct, but inefficient.
It was done with no regard for resource usage, such as memory, and it should improve in that regard.
The main resource hog is using string enum values which is unnecessary. Logic changes have to be done to make sure they are cleaned from the rest of the code.
Small resource usage in one function returning by reference, when it should return by value.
Finally, there are types that hold references to other types. This creates double indirection and might make the GC work more. Nonetheless, it is not straightforward to me in this first review whether those are wrong or right. We can discuss the details of this in the next daily
| type FeeEstimate struct { | ||
| GasTokenPriceInStrk *felt.Felt `json:"gas_token_price_in_strk"` | ||
| EstimatedFeeInStrk *felt.Felt `json:"estimated_fee_in_strk"` | ||
| EstimatedFeeInGasToken *felt.Felt `json:"estimated_fee_in_gas_token"` | ||
| SuggestedMaxFeeInStrk *felt.Felt `json:"suggested_max_fee_in_strk"` | ||
| SuggestedMaxFeeInGasToken *felt.Felt `json:"suggested_max_fee_in_gas_token"` | ||
| } |
There was a problem hiding this comment.
Are all of these fields optional, or are some of them optional?
|
@thiagodeev why is this not targetting main? |
thiagodeev
force-pushed
the
paymster-pr-2
branch
from
d140f83 to
debb1ae
Compare
thiagodeev
force-pushed
the
paymster-pr-2
branch
from
7ff68cf to
8079b1e
Compare
paymaster/build_txn.go
Outdated
| // - error: An error if the request fails | ||
| func (p *Paymaster) BuildTransaction( | ||
| ctx context.Context, | ||
| request BuildTransactionRequest, |
There was a problem hiding this comment.
Pass request by reference, please. There is no need to copy the entire value every time this function is called.
| if err := p.c.CallContextWithSliceArgs( | ||
| ctx, &response, "paymaster_buildTransaction", request, | ||
| ); err != nil { | ||
| return response, rpcerr.UnwrapToRPCErr( |
There was a problem hiding this comment.
I think is better to return a default value
return BuildTransactionResponse{}, rpcerr....The same way you return nil on an error when returning a reference, you return the default zero value in this case
There was a problem hiding this comment.
But if there's an error, the var response will be the zero value
| // An enum representing the Cairo version of the account contract | ||
| // to be deployed. Cairo 0 is not supported. | ||
| type CairoVersion int | ||
|
|
||
| const ( | ||
| // Represents the Cairo 1 version | ||
| Cairo1 CairoVersion = 1 | ||
| ) |
There was a problem hiding this comment.
Why have an enum of just one variant? If it is just one variant then is redundant holding information of it as far as I can see
There was a problem hiding this comment.
I'm just following the snip. Since it says it's an enum, I believe it's better to leave it as is.
https://github.com/starknet-io/SNIPs/blob/906b19074038005dc26d3574b6853a7c68d14e02/assets/snip-29/paymaster_api.json#L1128
| // An enum representing the version of the execution parameters | ||
| type UserParamVersion int | ||
|
|
||
| const ( | ||
| // Represents the v1 of the execution parameters ("0x1") | ||
| UserParamV1 UserParamVersion = iota + 1 | ||
| ) |
There was a problem hiding this comment.
Similar comment regarding this, are there more versions planned? Can you share the source?
There was a problem hiding this comment.
This is the snip-29 API spec.
It's an enum with a single option
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in [Nubia] Starknet.go
2 participants
Part 2 of the effort to break down the Paymaster PR for better review.
This PR:
paymaster_buildTransactionmethod