feat: compute max_weight for multisig example #402
Conversation
kratico
force-pushed
the
feat/multisig-example-fee-estimation
branch
from
b64ed4d to
5b80ea6
Compare
| .next((weight) => { | ||
| return { | ||
| ref_time: BigInt(weight.ref_time), | ||
| proof_size: BigInt(weight.proof_size), | ||
| } | ||
| }) |
There was a problem hiding this comment.
@tjjfvi Would you know if this is codec related?
ref_time and proof_size are u64 but they are not decoded as biging
There was a problem hiding this comment.
That weight seems to come from the RPC, which – being JSON – can only send numbers.
There was a problem hiding this comment.
you are right
It might be a good DX to have the codec convert number to bigint when it's needed, WDYT?
There was a problem hiding this comment.
Having any implicit coercion legitimizes implicit coercion in other cases, which could quickly get out of hand (c.f. @polkadot/types, which is a rat's nest of implicit coercion).
Additionally, all codecs from scale-ts currently roundtrip exactly for every valid input (that is,
!$.is($foo, value) || deepEqual(value, $foo.decode($foo.encode(value)))is always true), and I'd like to preserve this behavior.
There was a problem hiding this comment.
I think it would probably make the most sense for the .feeEstimate effect to do this BigInt coercion.
There was a problem hiding this comment.
if we do it in the effect, as soon as the weight shape changes, the effect will need to be refactored (like it happened with recent polkadot updates).
that's why I believe that implicit coercion from number to bigint will scale better for this case.
examples/multisig_transfer.ts
Outdated
| sender: C.compat.multiAddressFromKeypair(pair), | ||
| palletName: "Multisig", | ||
| methodName: "as_multi", | ||
| args: C.Z.rec({ | ||
| threshold: THRESHOLD, | ||
| call: { | ||
| type: "Balances", | ||
| value: { | ||
| type: "transfer_keep_alive", | ||
| dest: C.compat.multiAddressFromKeypair(T.dave), | ||
| value: 1_230_000_000_000n, | ||
| }, | ||
| }, | ||
| other_signatories: signatories.filter((value) => value !== pair.publicKey), | ||
| store_call: false, | ||
| max_weight: maxWeight, | ||
| maybe_timepoint: maybeTimepoint as Rest[0], |
There was a problem hiding this comment.
@harrysolovay the extrinsic data is almost the same but the spread operator doesn't work well with C.Z.rec
Would you know a better alternative?
I could factor out the args within Z.rec, WDYT?
There was a problem hiding this comment.
Or, given that it's an example, code duplication might be fine
There was a problem hiding this comment.
I think you want to calculate the weight for the Balances.transfer_keep_alive call, not the Multisig.as_multi, which would solve this duplication.
There was a problem hiding this comment.
Yea, I believe you'll be able to get the appropriate weight using the encoded inner extrinsic.
On a more general note though...
- A bit of redundancy in examples can definitely help with reader comprehension
- In this particular case, I suppose we could extract the common fields –– another way we'll make this example nicer: @tjjfvi is working on feat: next-gen codegen #368, which will enable us to generate/use narrow call data types (and their factory fns)
There was a problem hiding this comment.
Yeah, I'll resolve the duplication in the codegen branch using the call factories once this is merged – no need to address it now.
| @@ -38,7 +38,6 @@ | |||
| "test:update": "deno task test -- -- --update", | |||
| "mdbook:watch": "mdbook watch -o", | |||
| "bench": "deno bench -A --no-check=remote --unstable", | |||
| "multisig": "deno task run test_util/ctx.ts -- deno task run examples/multisig_transfer.ts", | |||
There was a problem hiding this comment.
When I committed the multisig example, I forgot to remove this task
Use
extrinsic.feeEstimationto compute multisigmax_weight