Skip to content
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

fix(ant): add priority as an attribute on ANTs, utility function for sorting algo #376

Merged
merged 7 commits into from
Feb 5, 2025

Conversation

dtfiedler
Copy link
Collaborator

@dtfiedler dtfiedler commented Feb 4, 2025

ANTs can optionally provide a priority order of their records. This is how ar-io gateways will enforce undername limits. If no priority order is provided, records will be sorted lexicographically.

This PR updates the type for ANRecords, and adds a utility to provide the proper sorting. We could extend this in the future to allow ar-io-nodes to choose their default sort behavior.

References:

@dtfiedler dtfiedler force-pushed the PE-7558-ant-priority-order branch from 0737d42 to db31a3b Compare February 4, 2025 20:27
@dtfiedler dtfiedler marked this pull request as ready for review February 4, 2025 20:27
@dtfiedler dtfiedler requested a review from a team as a code owner February 4, 2025 20:27
@dtfiedler dtfiedler requested a review from djwhitt February 4, 2025 20:51
@dtfiedler dtfiedler force-pushed the PE-7558-ant-priority-order branch from 3e5c21d to 66c5211 Compare February 4, 2025 20:52
@dtfiedler dtfiedler changed the title fix(ant): add priority as an attribute on ANTs fix(ant): add priority as an attribute on ANTs, utility function for sorting algo Feb 4, 2025
fedellen
fedellen previously approved these changes Feb 4, 2025
dtfiedler added 7 commits February 4, 2025 16:30
ANTs can optionally provide a priority order of their records. This is how ar-io gateways will enforce undername limits. If no priority order is provided, records will be sorted lexigraphically
Update unit tests to be closer to their respective implementations
@dtfiedler dtfiedler force-pushed the PE-7558-ant-priority-order branch from 8739bf8 to 3ad1d07 Compare February 4, 2025 22:30
@dtfiedler dtfiedler requested a review from fedellen February 4, 2025 22:40
@dtfiedler dtfiedler merged commit 8c6738f into alpha Feb 5, 2025
13 checks passed
});
export type AoANTRecord = z.infer<typeof AntRecordSchema>;
export type ANTRecords = Record<string, AoANTRecord>;
export type SortedANTRecord = AoANTRecord & { index: number };
export type SortedANTRecords = Record<string, SortedANTRecord>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It's nice when the key type is a type alias that describes the domain of the key string.

@dtfiedler
Copy link
Collaborator Author

🎉 This PR is included in version 3.5.0-alpha.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

describe('sortANTRecordsByPriority', () => {
it('should sort records by priority and then lexicographically', () => {
const records = {
undername1: { priority: 1, transactionId: 'test', ttlSeconds: 1 },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be insightful to have undername11 with priority 1 to ensure the lexicographical sort is working as expected.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And perhaps undername01

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in #385

// if both records have a priority, sort by priority and fallback to lexicographic sorting
if (aRecord.priority !== undefined && bRecord.priority !== undefined) {
if (aRecord.priority === bRecord.priority) {
return a.localeCompare(b);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: localeCompare's behavior can be overridden by the host platform's ENV vars like LC_ALL, LC_COLLATE, or LANG. Could lead to inconsistent results across user platforms.

});
});

it('should always return @ as the first record, regardless of priority', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem to be missing tests for mixed priority and no priority

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in #385

@dtfiedler
Copy link
Collaborator Author

🎉 This PR is included in version 3.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants