-
Notifications
You must be signed in to change notification settings - Fork 6
feat(transact): keyreg txn support #163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -116,6 +121,12 @@ impl AssetTransferTransactionBuilder { | |||
} | |||
} | |||
|
|||
impl KeyRegistrationTransactionBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we have all the builders defined here. Probably makes sense to move them into their respective files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good call.
pub non_participation: Option<bool>, | ||
} | ||
|
||
impl KeyRegistrationTransactionFields { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach. Perhaps we have a ValidTransaction
trait that has a valid
function with a default implementation that does checks on the header and the calls a valid_for_type
function that is called internally to do specific checks.
As an example (untested, may have errors):
trait ValidTransaction {
fn valid_for_type(&self) Result<(), String>;
fn valid(&self) Result<(), String> {
// Do checks on the header fields
self.valid_for_type()
}
}
impl ValidTransaction for KeyRegistrationTransaction {
fn is_valid_for_type(&self) Result<(), String> {
self.valid_offline()?
self.valid_online()
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickDinh Has done up an implementation of this in his PR https://github.com/algorandfoundation/algokit-core/pull/161/files
The naming is slightly different (trait Validate
with validate
) and there isn't any header validation as the header type mostly prevents invalid data being set, however that can be added like you have above very easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neilcampbell added trait Validate impl, ready for re review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, I approved by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joe-p Shout once you've updated based on the new conventions and we'll review.
pub non_participation: Option<bool>, | ||
} | ||
|
||
impl KeyRegistrationTransactionFields { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickDinh Has done up an implementation of this in his PR https://github.com/algorandfoundation/algokit-core/pull/161/files
The naming is slightly different (trait Validate
with validate
) and there isn't any header validation as the header type mostly prevents invalid data being set, however that can be added like you have above very easily.
Entirely written by claude
also generated by claude, not yet reviewed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds full support for Algorand key registration transactions across TypeScript, Python, Rust core, and FFI layers, including serialization, builder patterns, validation, and tests for online, offline, and non-participating modes.
- Added TypeScript tests and JSON parsing updates for key registration.
- Extended Python test data loader and examples for key registration.
- Introduced FFI-safe key registration types and updated test plans.
- Implemented core Rust
KeyRegistrationTransactionFields
, builder, validation, and polytest coverage.
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/typescript/algokit_transact/tests/key_registration.test.ts | New TS tests for key registration transactions |
packages/typescript/algokit_transact/tests/common.ts | Reviver update to convert numeric fields to BigInt |
packages/python/algokit_transact/tests/test_key_registration.py | Python example/tests for key registration |
packages/python/algokit_transact/tests/init.py | Extended test data loader for key registration |
crates/algokit_transact_ffi/src/transactions/keyreg.rs | FFI struct and conversions for key registration |
crates/algokit_transact_ffi/polytest.toml | Added key registration suite to test plan |
crates/algokit_transact/src/transactions/key_registration.rs | Core Rust implementation and validation logic |
packages/typescript/algokit_transact/__tests__/key_registration.test.ts
Outdated
Show resolved
Hide resolved
- Adds validation for key registration transactions to ensure that online key registration transactions have all required participation key fields. - Introduces validation to ensure vote_first is less than vote_last. - Prevents setting the non_participation flag for online key registration transactions. - Improves error reporting for key registration validation failures.
@@ -156,6 +159,31 @@ impl TransactionMother { | |||
.receiver(AddressMother::neil()) | |||
.to_owned() | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For asset config and app call we split these out into a seperate file. Are you able update to use the same pattern?
@@ -156,6 +159,31 @@ impl TransactionMother { | |||
.receiver(AddressMother::neil()) | |||
.to_owned() | |||
} | |||
|
|||
pub fn online_key_registration() -> KeyRegistrationTransactionBuilder { | |||
KeyRegistrationTransactionBuilder::default() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to use real data from testnet if you can, that way it's easy to verify that the transaction id matches correctly. App call has an example of this and also adds a lora link comment for reference.
@@ -410,3 +410,171 @@ fn test_signed_transaction_group_encoding() { | |||
assert_eq!(decoded_signed_tx, signed_grouped_tx); | |||
} | |||
} | |||
|
|||
#[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests should be in transactions/key_registration.rs file. You can follow how both app call and asset config does it. Additionally some convenience functions have been extracted to help with testing the encoding, which you can take a look at asset config and app call.
|
||
/// Mark account as non-reward earning. | ||
#[serde(rename = "nonpart")] | ||
#[serde(skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth confirming if serialization should be skipped when value is false as well?
} | ||
|
||
impl Validate for KeyRegistrationTransactionFields { | ||
fn validate(&self) -> Result<(), Vec<String>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the same patterns used in asset config, so that they inherit the same conventions and messaging.
Also given is_online
, is_offline
, is_non_participating
functions exists, we should probably use those to determine how to validate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also asset config uses a different approach which doesn't include is_online
etc. I think we should probably keep consistent.
@@ -0,0 +1,89 @@ | |||
//! FFI-safe key registration transaction types for AlgoKit Core. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//! FFI-safe key registration transaction types for AlgoKit Core. | |
//! Represents a key registration transaction. |
.to_owned() | ||
} | ||
|
||
pub fn non_participating_key_registration() -> KeyRegistrationTransactionBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we align all terminology to non_participation
? There is a few places to update.
pub fn non_participating_key_registration() -> KeyRegistrationTransactionBuilder { | |
pub fn non_participation_key_registration() -> KeyRegistrationTransactionBuilder { |
} | ||
|
||
impl Validate for KeyRegistrationTransactionFields { | ||
fn validate(&self) -> Result<(), Vec<String>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also asset config uses a different approach which doesn't include is_online
etc. I think we should probably keep consistent.
TODO
Summary
Entirely written by claude. Haven't yet reviewed anything, but wanted to commit and PR first so we can all see what it's first shot looked like.
- KeyRegistrationTransactionFields struct with all required fields:
- Builder pattern support via KeyRegistrationTransactionBuilder
- Integration with Transaction enum using the correct serde tag "keyreg"
- FFI support in the algokit_transact_ffi crate
- Creating online key registration transactions
- Updating (offline key registration transactions)
- Deleting (non-participating key registration)
- Calling in transaction groups with other transaction types
- Encoding/decoding round-trip tests
- Transaction ID generation
- Fee calculation
- Online/offline detection
Key Features Implemented:
The implementation follows the existing patterns in the codebase and includes comprehensive tests as requested. The KeyReg transaction type is now fully functional and ready to use in applications that need to manage Algorand account
participation in consensus.