chore: fix attestation readme and support numeric typing#137
Conversation
Time Submission Status
|
MicBun
changed the title
WalkthroughUpdates attestation example documentation with Quick Start approach using tsx instead of ts-node, adds default test private key support, updates kwil-js dependency to ^0.9.10, and implements proper type encoding for maxFee (wei) and BYTEA parameter handling in attestation operations. Changes
Sequence DiagramsequenceDiagram
actor User as User
participant SDK as SDK
participant KwilJS as kwil-js
participant Contract as Contract
User->>SDK: requestAttestation(maxFee: number)
activate SDK
rect rgb(220, 240, 250)
Note over SDK: Type Conversion Phase
SDK->>SDK: Convert maxFee to string (wei)
SDK->>SDK: Create types mapping:<br/>{ $max_fee: 'numeric(78,0)' }
end
rect rgb(240, 250, 220)
Note over SDK: Execution Phase
SDK->>SDK: executeWithNamedParams(method,<br/>inputs, types)
end
SDK->>KwilJS: execute(request with types)
activate KwilJS
KwilJS->>Contract: Submit attestation<br/>(proper type encoding)
activate Contract
Contract-->>KwilJS: Receipt
deactivate Contract
KwilJS-->>SDK: TxReceipt
deactivate KwilJS
SDK-->>User: Success
deactivate SDK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/contracts-api/action.ts (1)
94-111: LGTM! Optional types parameter enables proper numeric encoding.The new parameter allows specifying types like
NUMERIC(78,0)for large wei amounts, which is essential for attestation fees. The optional parameter maintains backward compatibility.Consider tightening the type signature to leverage kwil-js types:
- types?: Record<string, any>, + types?: Record<string, Types.ValueType>,This would provide better type safety if kwil-js exports the appropriate type.
examples/attestation/README.md (1)
38-38: Fix grammar for improved readability.Add a comma before "but" since it connects two independent clauses.
Apply this diff:
- **Note**: If `PRIVATE_KEY` is not set, the example will use a default test private key (`0x000...001`). This is useful for testing but the wallet may not have sufficient balance on mainnet. + **Note**: If `PRIVATE_KEY` is not set, the example will use a default test private key (`0x000...001`). This is useful for testing, but the wallet may not have sufficient balance on mainnet.src/types/attestation.ts (1)
194-203: LGTM! Validation correctly handles multiple numeric types.The conversion logic properly handles all three input types and performs BigInt comparison for accuracy.
Note:
Math.flooron line 199 could theoretically lose precision for numbers exceedingNumber.MAX_SAFE_INTEGER. Since the type already acceptsbigint, users should prefer passing large values asbigintorstringfor precision. Consider documenting this in the JSDoc formaxFee.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/attestation/README.md(2 hunks)examples/attestation/index.ts(2 hunks)package.json(1 hunks)src/contracts-api/action.ts(1 hunks)src/contracts-api/attestationAction.ts(4 hunks)src/types/attestation.ts(2 hunks)
🧰 Additional context used
🪛 LanguageTool
examples/attestation/README.md
[uncategorized] ~38-~38: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...x000...001`). This is useful for testing but the wallet may not have sufficient bala...
(COMMA_COMPOUND_SENTENCE_2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (10)
package.json (1)
54-54: LGTM! Dependency update aligns with new kwil-js features.The bump to
^0.9.10supports the newUtils.DataTypeAPI and enhanced parameter typing used in attestation operations.examples/attestation/README.md (2)
42-62: LGTM! Documentation aligns with Quick Start flow.The updated instructions using
tsxand the default test key make the example more accessible for initial testing.
206-209: LGTM! Helpful guidance on attestation fees.The updated error section clearly mentions the 40 TRUF fee requirement and advises setting appropriate
maxFeevalues.examples/attestation/index.ts (2)
22-28: LGTM! Helpful default for initial testing.The default test private key with clear warnings improves the developer experience while guiding users to set their own key for production use.
73-73: Verify the attestation fee amount is correct.The
maxFeeis set to"40000000000000000000"wei (40 TRUF). Ensure this aligns with the actual attestation fee structure.The string representation correctly handles the large wei amount and aligns with the updated type signature that accepts
number | string | bigint.src/types/attestation.ts (1)
44-48: LGTM! Type widening supports large wei amounts.Accepting
number | string | bigintenables proper handling of attestation fees up to 40 TRUF (40e18 wei), which exceeds JavaScript's safe integer range.src/contracts-api/attestationAction.ts (4)
9-9: LGTM! Utils import enables numeric type specification.The
Utilsimport provides access toDataType.Numeric()for properly encoding large wei amounts.
95-111: LGTM! Proper numeric encoding for large wei amounts.The implementation correctly:
- Converts
maxFeeto string for safe handling- Specifies
NUMERIC(78,0)type for up to 78-digit wei values- Passes types to
executeWithNamedParamsThis ensures attestation fees are accurately encoded without precision loss.
272-274: LGTM! Defensive normalization for result value.The code safely handles cases where
result.valuemight be a getter function, ensuring consistent array processing.
253-259: The referenced version kwil-js 0.9.10+ does not exist.The latest published version of the official kwil-js package is 0.9.4, not 0.9.10. The review comment's claim about kwil-js 0.9.10+ handling empty
Uint8ArrayasBYTEA NULLcannot be verified against a non-existent version. The actual behavior of emptyUint8Arrayfor BYTEA parameters in the current version (0.9.4) should be verified separately against official documentation or tested directly, rather than relying on the unsubstantiated version reference in the code comment.Likely an incorrect or invalid review comment.
|
@MicBun CI failed due to max fee config on tests |
|
Yes, currently no whitelist, i am creating it now |
got it |
2 participants
resolves: #136
Summary by CodeRabbit
Documentation
Improvements