Skip to content

Conversation

@lla-dane
Copy link
Contributor

Tracks multiformats/multiaddr#181

Added the memory codec in py-multiaddr in reference with go-multiaddr implementation of memory:
https://github.com/multiformats/go-multiaddr/blob/b2ad16d978ea6b0f2cc49d20da2c0db24c92063d/transcoders.go#L493C1-L520C2

@lla-dane
Copy link
Contributor Author

@acul71 @seetadev @pacrob: Please take a look at this, and see if its correctly done. The make lint command was working after the new install branch merge.

acul71 added a commit to lla-dane/py-multiaddr that referenced this pull request Sep 15, 2025
- Changed from codec=None to codec='memory'
- Now properly uses the memory codec for encoding/decoding
- Fixes the issue mentioned in PR multiformats#92 where the author was unsure about the codec parameter
- Follows the established pattern used by other protocols with custom codecs
@acul71
Copy link
Contributor

acul71 commented Sep 15, 2025

Hi @lla-dane . just add some tests and I'll merge it

PR #92 Review: Memory Protocol Implementation

Overview

This PR adds support for the memory protocol (code 777) to py-multiaddr, implementing a uint64-based memory address system.

✅ What's Working Well

  • Codec Implementation: Well-structured memory.py with proper uint64 big-endian encoding
  • Error Handling: Comprehensive validation for range and format errors
  • Protocol Registration: Correctly added to PROTOCOLS list
  • Codec Registration: Fixed from None to "memory" (was the main issue)
  • Unit Tests: Excellent coverage in tests/test_codec.py (edge cases, errors, roundtrip)

❌ Missing Tests - Critical Gaps

1. Integration Tests Missing

The memory protocol is only tested in isolation. Missing tests in main test suite:

# Missing from tests/test_multiaddr.py
def test_memory_protocol_integration():
    ma = Multiaddr('/memory/12345')
    assert str(ma) == '/memory/12345'
    assert ma.protocols()[0].name == 'memory'
    assert ma.value_for_protocol(777) == '12345'
    
    # Binary roundtrip
    binary = ma.to_bytes()
    reconstructed = Multiaddr(binary)
    assert str(reconstructed) == str(ma)

2. Protocol Property Tests Missing

No tests for memory protocol-specific behavior:

# Missing from tests/test_protocols.py
def test_memory_protocol_properties():
    proto = Protocol(P_MEMORY, "memory", "memory")
    assert proto.size == 8  # uint64 size
    assert proto.path == False  # not a path protocol
    assert proto.code == 777
    assert proto.name == "memory"

🔧 Suggested Test Additions

Add to tests/test_multiaddr.py:

def test_memory_protocol_basic():
    """Test basic memory protocol functionality"""
    ma = Multiaddr('/memory/12345')
    assert str(ma) == '/memory/12345'
    assert len(ma.protocols()) == 1
    assert ma.protocols()[0].name == 'memory'
    assert ma.protocols()[0].code == 777

def test_memory_protocol_value_extraction():
    """Test memory protocol value extraction"""
    ma = Multiaddr('/memory/12345')
    assert ma.value_for_protocol(777) == '12345'
    assert ma.value_for_protocol(4) is None  # wrong protocol

def test_memory_protocol_binary_roundtrip():
    """Test memory protocol binary encoding/decoding"""
    ma = Multiaddr('/memory/12345')
    binary = ma.to_bytes()
    reconstructed = Multiaddr(binary)
    assert str(reconstructed) == str(ma)

def test_memory_protocol_edge_cases():
    """Test memory protocol edge cases"""
    # Min value
    ma_min = Multiaddr('/memory/0')
    assert ma_min.value_for_protocol(777) == '0'
    
    # Max value
    ma_max = Multiaddr('/memory/18446744073709551615')
    assert ma_max.value_for_protocol(777) == '18446744073709551615'

def test_memory_protocol_invalid_inputs():
    """Test memory protocol invalid inputs"""
    with pytest.raises(StringParseError):
        Multiaddr('/memory/-1')
    with pytest.raises(StringParseError):
        Multiaddr('/memory/abc')
    with pytest.raises(StringParseError):
        Multiaddr('/memory/18446744073709551616')  # overflow

Add to tests/test_protocols.py:

def test_memory_protocol_properties():
    """Test memory protocol properties"""
    proto = Protocol(P_MEMORY, "memory", "memory")
    assert proto.size == 8
    assert proto.path == False
    assert proto.code == 777
    assert proto.name == "memory"
    assert proto.codec == "memory"

🎯 Priority Recommendations

High Priority

  1. Add basic integration tests to test_multiaddr.py
  2. Add protocol property tests to test_protocols.py

Medium Priority

  1. Add edge case tests for min/max values in integration context
  2. Add error handling tests for invalid inputs in integration context

📋 Test Checklist

  • Basic memory protocol creation and string representation
  • Protocol extraction and property access
  • Value extraction using value_for_protocol()
  • Binary encoding/decoding roundtrip
  • Edge cases (0, max uint64) in integration context
  • Error handling (negative, overflow, invalid format) in integration context
  • Protocol properties (size, path, code, name)

💡 Implementation Hints

  1. Follow existing patterns in test_multiaddr.py for similar protocols
  2. Use pytest.raises() for error testing
  3. Test both string and binary creation methods
  4. Verify protocol properties match expected values

The current implementation is solid, but adding these integration tests would make it production-ready! 🚀

@seetadev
Copy link
Contributor

@lla-dane : Great efforts, Abhinav.

The linting issues have been resolved.

Wonderful to see the addition of memory protocol.

We will follow this structure to add more protocols. Lets have 1 PR per protocol first while we test a couple of additions in py-libp2p. We can then go for one major PR with remaining protocol additions.

@seetadev
Copy link
Contributor

@lla-dane : Please add more test cases as shared by @acul71 above. Also, add the newsfragment. Great initiative indeed. Appreciate your efforts and support.

Will ask @acul71 and @pacrob for review once the initial test suite is ready and newsfragment added.

@lla-dane
Copy link
Contributor Author

@acul71 @seetadev Added the recommended test cases. Please take a look and approve the CI/CD runs.

@seetadev
Copy link
Contributor

@lla-dane : Thank you Abhinav for considering all our feedback points. Appreciate your efforts.

The test suite is looking near. Reviewing it in details.

Re-ran the CI/CD pipeline, which is in progress.

The PR is ready for final review + merge. Will review in details today and merge the PR.

In the meantime, you could open PRs for adding other key protocols - you could have 1-2 per PR. This would enable testing in parallel in py-libp2p front as well.

On the same note, please initially pick up the protocols, which you can test at your end in py-libp2p. This would enable us to scale efforts quickly.

@seetadev
Copy link
Contributor

@lla-dane : This looks ready for merge. Please add a discussion page in py-libp2p repository covering the implementation steps for adding the memory protocol in py-multiaddr. Appreciate your efforts.

@seetadev seetadev merged commit 0f8b6c0 into multiformats:master Sep 15, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants